[ptr727]

Summary

Refs #1139.

• Every delete of user content goes through a new FileDelete helper. It logs Deleting {What} : {Reason} : {FileName} before it acts, and Failed to delete {What} : {FileName} as an Error when the delete fails.
  • The sites are the orphan sidecar, the non-MKV file, the failed media file and its sidecar, the input a conversion replaced, and each empty folder.
  • DeleteFailedFile takes the failed operation, so its reason reads Failed VerifyAndRepair rather than a generic line.
  • The sidecar removed with a failed media file logs its own delete line, only when a sidecar exists.
  • The empty-folder delete logs each folder it removes, in addition to the total. A failed folder delete logs, continues to the next folder, and still makes DeleteEmptyFolders return false as before.
  • A failed orphan-sidecar or non-MKV delete logs the named Error and rethrows, so Process.ProcessFile still fails the file without deleting anything else, as before.
• With DeleteInvalidFiles on, DeleteFailedFile logs Keeping failed file : {Reason} : {FileName} for each keep. The reason is one of rename collision, tool error, tool signal, cancelled, or PostDecodeTimestamps.
• A media-file delete is a Warning, while a sidecar, folder, or converted-input delete is Information. The issue proposed Warning for every delete. ARCHITECTURE.md "Logging Conventions" reserves Warning for the decision to modify media and keeps it out of Convert and sidecar bookkeeping, so the committed taxonomy decides the level. The new "Delete and keep logging" subsection records this.
• A read-only file or an exception never reaches DeleteFailedFile, and its own Error line already says why the file is kept. The docs name only the keeps DeleteFailedFile logs.
• Convert.DeleteTempFile is left alone, since a temp file is not user content.

Tests

• MkvFile_LogsOperationAndSidecarDelete asserts the media Warning names the failed operation, and the sidecar delete line.
• ToolError_LogsKeepReason asserts the keep line and that the file stays.
• SidecarDeleteThrows_LogsFailedDelete and NonMkvDeleteThrows_LogsFailedDeleteAndPropagates use a directory in place of a file to fail the delete. The second asserts the Error and the rethrow.
• DeleteEmptyFolders_LogsEachFolder asserts one line per removed folder, innermost first.
• The existing DeleteFailedFile() callers pass nameof(ProcessFile.VerifyAndRepair).
• All tests pass locally, none skipped.

Local Review

Three recorded local-strict-review passes ran. The first raised three introduced findings: docs promising a keep line for read-only and exception cases, the delete shape repeated per site instead of one helper, and an untested rethrow path. The second raised one: the converted-input reason carried the output path unquoted. Each is fixed on this branch.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 31 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: faa3d782-20a7-4cb7-b29a-e0f56a04b571

Commits

Reviewing files that changed from the base of the PR and between 7d1ccbc and b7a86cf.

Files selected for processing: 11

• ARCHITECTURE.md
• HISTORY.md
• PlexCleaner/Convert.cs
• PlexCleaner/FileDelete.cs
• PlexCleaner/Process.cs
• PlexCleaner/ProcessFile.cs
• PlexCleanerTests/DeleteFailedFileTests.cs
• PlexCleanerTests/LogCapture.cs
• PlexCleanerTests/ReEncodeBitrateStateTests.cs
• PlexCleanerTests/RenameCollisionTests.cs
• PlexCleanerTests/VerifyToolErrorTests.cs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[ptr727]

A recorded local strict-review pass covers head d9f026cef632bb0913a3162476b3cd70e3a0ad34, the content this pull request carries at that commit against develop, and it recorded 0 findings.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Log reasons for user-content deletes and failed-file keeps

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Route user-content deletes through one helper for consistent reasons and failure logs.
• Log why failed files are kept when DeleteInvalidFiles is enabled.
• Cover delete failures and log levels with tests, and document the conventions.

Diagram

graph TD
  A["ProcessFile"] --> B{"Keep failed?"} -->|yes| C["Keep reason log"]
  B -->|delete| D["FileDelete"] --> F["Delete log"]
  E["Convert"] --> D
  G["Folder cleanup"] --> D
  D -->|failure| H["Failure log"]

Loading

High-Level Assessment

The shared helper is appropriate: it keeps the log format consistent while letting callers choose whether a failed delete is caught or propagated. Duplicating the logging at each site would make those messages easier to drift.

Files changed (9) +298 / -62

Enhancement (4) +119 / -48

Convert.csLog deletion of replaced conversion inputs +7/-11

Log deletion of replaced conversion inputs

• Routes converted-input cleanup through FileDelete with the output path as its reason. Temporary-file cleanup remains unchanged.

PlexCleaner/Convert.cs

FileDelete.csCentralize user-content deletion logging +45/-0

Centralize user-content deletion logging

• Adds deletion methods that log the target and reason before acting and name the target on failure. Callers can either receive a false result or let the exception propagate.

PlexCleaner/FileDelete.cs

Process.csPass failed operations and log each empty-folder delete +22/-6

Pass failed operations and log each empty-folder delete

• Passes the failed processing operation into DeleteFailedFile. Empty-folder cleanup now logs each attempted deletion, continues after a failed folder delete, and reports failure in its return value.

PlexCleaner/Process.cs

ProcessFile.csLog file deletions and reasons for retained failures +45/-31

Log file deletions and reasons for retained failures

• Routes orphan-sidecar, non-MKV, failed-media, and associated-sidecar deletes through FileDelete. When DeleteInvalidFiles is enabled, eligible failed files that cannot be deleted instead receive a specific keep reason.

PlexCleaner/ProcessFile.cs

Tests (3) +168 / -14

DeleteFailedFileTests.csTest delete, keep, and failure logs +157/-3

Test delete, keep, and failure logs

• Adds assertions for operation-specific media logs, sidecar deletion, tool-error keeps, failed-delete logging and propagation, and per-folder log order. Restores additional shared configuration after each test.

PlexCleanerTests/DeleteFailedFileTests.cs

ReEncodeBitrateStateTests.csPass the failed operation in bitrate tests +1/-1

Pass the failed operation in bitrate tests

• Updates the DeleteFailedFile call to supply the required operation name while retaining its existing assertion.

PlexCleanerTests/ReEncodeBitrateStateTests.cs

VerifyToolErrorTests.csPass the failed operation in verification tests +10/-10

Pass the failed operation in verification tests

• Updates DeleteFailedFile calls across tool-error and repair scenarios to supply the required operation name. Existing file-retention assertions remain in place.

PlexCleanerTests/VerifyToolErrorTests.cs

Documentation (2) +11 / -0

ARCHITECTURE.mdDefine delete and keep logging conventions +9/-0

Define delete and keep logging conventions

• Documents message formats, log levels, and failure behavior for user-content deletes. Clarifies which failed-file keeps receive a keep-reason line.

ARCHITECTURE.md

HISTORY.mdRecord delete and keep logging changes +2/-0

Record delete and keep logging changes

• Notes that deletes now log their reasons and failures name the affected file. Also records keep-reason logging for eligible DeleteInvalidFiles cases.

HISTORY.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Sidecar deletion failures go unreported 🐞 Bug ◔ Observability

Description

DeleteFailedFile gates sidecar cleanup on Path.Exists, which can return false for an
inaccessible path as well as a missing one, bypassing FileDelete.TryDelete. After the media file
is deleted, an inaccessible sidecar can remain while the method returns success without logging the
attempted deletion or its failure.

Code

PlexCleaner/ProcessFile.cs[2071]

+            if (Path.Exists(sidecarName))

Evidence

The new existence guard bypasses the only sidecar delete and its failure handler. The method sets
the deleted state before that guard and returns true regardless of whether sidecar cleanup ran.

PlexCleaner/ProcessFile.cs[2051-2081]
PlexCleaner/FileDelete.cs[14-23]
PlexCleaner/FileDelete.cs[33-43]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Path.Exists` can hide a sidecar that cannot be inspected, preventing the delete attempt and failure log after its media file is removed.
## Fix Focus Areas
- PlexCleaner/ProcessFile.cs[2068-2081]
## Recommended Fix
Distinguish a genuinely missing sidecar from an inaccessible one. Skip cleanup only when absence is established; otherwise attempt deletion through `FileDelete.TryDelete` so failures are logged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Delete exception logs no longer name the caller ✓ Resolved

Description

FileDelete.TryDelete catches with Log.Logger.LogAndHandle(e), and that method fills its
function argument from [CallerMemberName], so the exception's Error line now always reads
TryDelete instead of ReplaceInput, DeleteFailedFile or DeleteEmptyDirectories as it did
before. A failed delete of a media file, a sidecar, a converted input or a folder therefore logs a
stack-traced Error that is attributed to the shared helper, and the operator has to find the
separate Failed to delete {What} line to learn which site failed.

Code

PlexCleaner/FileDelete.cs[R19-21]

+        catch (Exception e) when (Log.Logger.LogAndHandle(e))
+        {
+            return false;

Evidence

LogAndHandle logs {Function} from [CallerMemberName], and the caller is now always the helper.
Before this change each site called LogAndHandle from inside its own method (for example, the
removed try/catch in Convert.ReplaceInput and in ProcessFile.DeleteFailedFile).

PlexCleaner/Extensions.cs[16-23]
PlexCleaner/FileDelete.cs[7-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
FileDelete.TryDelete calls Log.Logger.LogAndHandle(e), so [CallerMemberName] always resolves to "TryDelete" and the real calling site is lost from the exception log.
## Fix Focus Areas
- PlexCleaner/FileDelete.cs[7-24]
## Recommended Fix
Add a `[System.Runtime.CompilerServices.CallerMemberName] string function = "unknown"` parameter to TryDelete (after `delete`), then call `Log.Logger.LogAndHandle(e, function)` so the original caller (ReplaceInput, DeleteFailedFile, DeleteEmptyDirectories) is recorded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[ptr727]

A recorded local strict-review pass covers head e01ed7690db748175f1f72f007fa0f36bf345550, the content this pull request carries at that commit against develop, and it recorded 0 findings.

[ptr727]

Answering Qodo's review findings:

1. Sidecar deletion failures go unreported

Declined with evidence in the ProcessFile.cs:2071 thread. The guard runs only after the media file in the same directory was deleted, so a present sidecar cannot read as missing for access reasons on Linux or Windows.

2. Delete exception logs no longer name the caller

Fixed in e01ed76. TryDelete passes its [CallerMemberName] caller to LogAndHandle.

[Copilot]

🟡 Changes recommended

The change still has three active nit-level issues: an inaccurate release-note claim, an ungrammatical history entry, and incomplete log coverage for the helper’s keep-reason branches.

3 open issues

• Limit the history claim to user-content deletes · New
• Clarify that DeleteInvalidFiles keeps the failed file · New
• Test every failed-file keep reason · New

What changed in this PR

This pull request standardizes logging for user-content deletions and failed-file retention, while updating related processing code, tests, and documentation.

Changes:

• Adds the FileDelete helper for consistent deletion, failure, and severity logging.
• Records operation-specific reasons when failed files are kept.
• Updates cleanup callers, regression tests, architecture guidance, and release history.
• Logs per-folder deletion outcomes and converted-input cleanup.

This pull request requires changes.

File	Description
PlexCleanerTests/​VerifyToolErrorTests.cs	Updates failed-file cleanup calls with an operation name.
PlexCleanerTests/​ReEncodeBitrateStateTests.cs	Updates failed-file cleanup calls with the operation name.
PlexCleanerTests/​DeleteFailedFileTests.cs	Adds coverage for deletion, keep reasons, failures, and empty-folder logging.
PlexCleaner/​ProcessFile.cs	Routes user-file deletions through FileDelete and logs failed-file keep reasons.
PlexCleaner/​Process.cs	Passes failed operations and tracks per-folder deletion failures.
PlexCleaner/​FileDelete.cs	Provides centralized deletion and failure logging behavior.
PlexCleaner/​Convert.cs	Routes converted-input deletion through the shared helper.
HISTORY.md	Documents the new deletion and keep logging behavior.
ARCHITECTURE.md	Defines deletion logging levels, formats, and failure semantics.

_{Review effort: Lite}

[ptr727]

A recorded local strict-review pass covers head b7a86cf5923bc5c64a3ae8a3e727f2dd92916a0c, the content this pull request carries at that commit against develop, and it recorded 0 findings.