Fix incomplete plugin directory deletion on uninstall#5
Conversation
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
|
@copilot I think we can do it better. If this directory is not fully deleted, I think we can add back delete file |
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Good suggestion! Implemented in commit f3f9d62. Now when the directory is not fully deleted, the |
Refactored the process for recreating the marker file when a plugin directory is not fully deleted. Removed unnecessary try-catch and nested checks, now directly checking for file existence and creating it if missing. This streamlines the code and removes redundant error handling.
There was a problem hiding this comment.
Pull request overview
This pull request addresses incomplete plugin directory deletion during uninstallation when files are locked by processes or have permission issues. The previous atomic deletion approach (Directory.Delete(directory, true)) would fail completely if any file was locked, leaving the plugin directory partially intact with empty subdirectories.
Changes:
- Introduced a new
TryDeleteDirectoryRobustmethod with individual file deletion, retry logic, and incremental cleanup - Updated plugin cleanup logic to use the robust deletion method and recreate the deletion marker on incomplete deletions
- Added comprehensive test coverage for the new deletion method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs | Implements TryDeleteDirectoryRobust with retry logic for locked files, read-only attribute handling, and bottom-up directory cleanup |
| Flow.Launcher.Core/Plugin/PluginConfig.cs | Integrates the robust deletion method and recreates the deletion marker when cleanup is incomplete to enable automatic retry on next startup |
| Flow.Launcher.Test/FilesFoldersTest.cs | Adds unit tests for various deletion scenarios including empty directories, nested structures, and read-only files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var file in files) | ||
| { | ||
| bool fileDeleted = false; | ||
| for (int attempt = 0; attempt <= maxRetries; attempt++) |
There was a problem hiding this comment.
Off-by-one error in retry logic. The loop condition 'attempt <= maxRetries' with 'attempt' starting at 0 means that when maxRetries is 3, the code will make 4 attempts (0, 1, 2, 3) instead of the intended 3. Change the loop condition to 'attempt < maxRetries' to ensure the number of attempts matches the parameter value, or adjust the documentation to clarify that maxRetries represents the number of retries after the initial attempt.
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryDoesNotExist_ReturnsTrue() | ||
| { | ||
| // Arrange | ||
| string nonExistentPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(nonExistentPath); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryIsEmpty_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryHasFiles_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| File.WriteAllText(Path.Combine(tempDir, "test.txt"), "test content"); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryHasNestedStructure_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| string subDir1 = Path.Combine(tempDir, "SubDir1"); | ||
| string subDir2 = Path.Combine(tempDir, "SubDir2"); | ||
| Directory.CreateDirectory(subDir1); | ||
| Directory.CreateDirectory(subDir2); | ||
| File.WriteAllText(Path.Combine(subDir1, "file1.txt"), "content1"); | ||
| File.WriteAllText(Path.Combine(subDir2, "file2.txt"), "content2"); | ||
| File.WriteAllText(Path.Combine(tempDir, "root.txt"), "root content"); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenFileIsReadOnly_RemovesAttributeAndDeletes() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| string filePath = Path.Combine(tempDir, "readonly.txt"); | ||
| File.WriteAllText(filePath, "readonly content"); | ||
| File.SetAttributes(filePath, FileAttributes.ReadOnly); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } |
There was a problem hiding this comment.
The test coverage doesn't include a test case for partial deletion scenarios where TryDeleteDirectoryRobust returns false. Consider adding a test that verifies the method returns false when file deletion fails (e.g., by mocking or using a test helper that simulates a locked file). This would ensure the core functionality that differentiates this method from Directory.Delete is properly tested.
Plugin uninstallation left empty directories with orphaned subdirectories when files were locked by processes or had permission issues.
Directory.Delete(directory, true)fails atomically—if any file is locked, nothing gets deleted.Changes
New deletion strategy (
FilesFolders.TryDeleteDirectoryRobust)boolindicating complete vs. partial deletionUpdated plugin cleanup (
PluginConfig.cs)NeedDelete.txtmarker on incomplete deletion - ensures automatic retry on next startup for eventual consistencyTest coverage (
FilesFoldersTest.cs)Example
Before:
After:
Handles both reported errors:
IOException: The process cannot access the file 'alarm.wav'UnauthorizedAccessException: Access to the path '...\Images' is deniedIf files remain locked after retry attempts, the deletion marker is preserved and cleanup is automatically retried on subsequent application restarts until successful.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.