Fix Directory_Delete_MountVolume test: guard Unmount calls with Directory.Exists check#125348
Merged
danmoseley merged 2 commits intomainfrom Mar 11, 2026
Merged
Fix Directory_Delete_MountVolume test: guard Unmount calls with Directory.Exists check#125348danmoseley merged 2 commits intomainfrom
danmoseley merged 2 commits intomainfrom
Conversation
…tory.Exists check Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix unmount calls in Directory_Delete_MountVolume tests
Fix Directory_Delete_MountVolume test: guard Unmount calls with Directory.Exists check
Mar 9, 2026
Contributor
|
Tagging subscribers to this area: @agocke |
This was referenced Mar 10, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes intermittent failures in the Directory_Delete_MountVolume Windows/NTFS test by avoiding cleanup exceptions when the mount point directory has already been removed by Directory.Delete.
Changes:
- Guard
MountHelper.Unmount(mountedDirName)andDeleteDir(mountedDirName, true)in Scenarios 1, 2, 3.1, and 3.2 withDirectory.Exists(mountedDirName). - Align cleanup behavior with the existing “only unmount if the mount point still exists” pattern already used in later scenarios.
2 tasks
ViveliDuCh
approved these changes
Mar 10, 2026
Member
ViveliDuCh
left a comment
There was a problem hiding this comment.
LGTM but I'm still new to this area and I was only able to validate x64 scenarios (not ARM64) with a repro that simulates the gone-mount-point condition.
Thanks! @jozkee
danmoseley
approved these changes
Mar 11, 2026
Copilot AI
added a commit
that referenced
this pull request
Mar 13, 2026
…tory.Exists check (#125348) `finally` blocks in Scenarios 1, 2, 3.1, and 3.2 unconditionally called `MountHelper.Unmount(mountedDirName)`. When `Directory.Delete` removes the mount point directory as part of the delete operation, the subsequent `Unmount` call throws (Win32 error 4390/3), gets caught by the scenario's `catch`, and sets `s_pass = false`. ## Changes - **`Delete_MountVolume.cs` — Scenarios 1, 2, 3.1, 3.2 `finally` blocks**: Wrap `Unmount`/`DeleteDir` calls with `Directory.Exists(mountedDirName)`, matching the pattern already used in Scenarios 3.3–3.5. ```csharp // Before finally { MountHelper.Unmount(mountedDirName); DeleteDir(mountedDirName, true); } // After finally { if (Directory.Exists(mountedDirName)) { MountHelper.Unmount(mountedDirName); DeleteDir(mountedDirName, true); } } ``` <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ## Problem The `Directory_Delete_MountVolume.RunTest()` test in `src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/Directory/Delete_MountVolume.cs` fails because the `finally` blocks in Scenarios 1, 2, 3.1, and 3.2 unconditionally call `MountHelper.Unmount(mountedDirName)`, but `Directory.Delete` may have already removed the mount point directory. When `Unmount` tries to call `DeleteVolumeMountPoint` on a path that no longer exists, it throws (Win32 error 4390 or 3), which gets caught by the scenario's `catch` block and sets `s_pass = false`, failing the test. Scenarios 3.3, 3.4, and 3.5 already correctly guard their cleanup with `if (Directory.Exists(mountedDirName))` before attempting to unmount. The fix is to apply the same pattern to the 4 `finally` blocks that are missing it. ## Fix Change the `finally` blocks in Scenarios 1, 2, 3.1, and 3.2 to guard the `Unmount` and `DeleteDir` calls with `Directory.Exists(mountedDirName)`, matching the existing pattern used in Scenarios 3.3–3.5. There are exactly 4 `finally` blocks to change. Each one currently looks like: ```csharp finally { MountHelper.Unmount(mountedDirName); DeleteDir(mountedDirName, true); } ``` Each should be changed to: ```csharp finally { if (Directory.Exists(mountedDirName)) { MountHelper.Unmount(mountedDirName); DeleteDir(mountedDirName, true); } } ``` The 4 locations in the file `src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/Directory/Delete_MountVolume.cs` are: 1. **Scenario 1 finally** (around lines 80–84) 2. **Scenario 2 finally** (around lines 125–129) 3. **Scenario 3.1 finally** (around lines 167–171) 4. **Scenario 3.2 finally** (around lines 209–213) **Do not change anything else.** No changes to `s_pass`, `Eval`, method signatures, test structure, or any other lines. Only add the `Directory.Exists` guard to these 4 `finally` blocks. The following is the prior conversation context from the user's chat exploration (may be truncated): User: explain this test to me and what s_pass asserts Assistant: ## Explanation of `Delete_MountVolume.RunTest()` This is a Windows-specific test that verifies `Directory.Delete` works correctly when **NTFS volume mount points** are involved. A volume mount point is a directory that acts as an entry point to another volume (similar to a drive letter, but mapped to a folder). ### What It Tests The test runs **7 scenarios**, all variations of the same theme — deleting directories that are accessed through or contain mounted volumes: | Scenario | Description | |----------|-------------| | **1** | A *different* drive is mounted onto the current drive. Delete a directory on the other drive *via* the mount point. | | **2** | The *current* drive is mounted onto a different drive. Delete a directory on the current drive *via* the mount point on the other drive. | | **3.1** | The current drive is mounted onto *itself* (at the root). Delete a directory via that self-referencing mount. | | **3.2** | The current drive is mounted onto the *current working directory*. Delete via that mount. | | **3.3** |⚠️ Delete the **mounted directory itself** (the mount point). Verifies the mount point is removed without nuking the underlying volume. | | **3.4** |⚠️ Delete the **parent directory** of a mount point (parent also has other files/dirs). Verifies recursive delete handles mount points inside the tree. | | **3.5** | Same as 3.4 but targets a *different* subdirectory as the mount point within the tree. | ### The Pattern in Each Scenario 1. **Create** a directory to serve as a mount point. 2. **Mount** a volume onto that directory using `MountHelper.Mount`. 3. **Create** a directory tree (files and subdirectories) on the source volume. 4. **Delete** the tree — either via the mount path or by deleting a parent that contains the mount. 5. **Verify** the directory no longer exists. 6. **Clean up** — unmount and delete the mount directory in a `finally` block. ### What `s_pass` Asserts `s_pass` is a **boolean accumulator** for the overall test result. It starts as `true` and is set to `false` in two situations: 1. **Any `Eval(...)` call returns `false`** — `Eval` checks a condition (e.g., "directory was deleted") and sets `s_pass = false` if the condition fails, logging the error. 2. **Any scenario throws an unhandled exception** — each `catch` block sets `s_pass = false` and logs the error. At the very end, in the outermost `finally`: ```csharp Assert.True(s_pass); ``` This is a **single assertion that summarizes all scenarios**. If *any* scenario failed a condition or threw an exception, `s_pass` will be `false` and the test fails. This pattern was common in older test code — it lets the test run all scenarios even if one fails (rather than stopping at the first `Assert` failure), collecting all errors before reporting. ### Why It's Disabled The `[ActiveIssue("https://github.com/dotnet/runtime/issues/14378")]` attribute means this... </details> <!-- START COPILOT CODING AGENT SUFFIX --> *This pull request was created from Copilot chat.* > <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jozkee <16040868+jozkee@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
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.
finallyblocks in Scenarios 1, 2, 3.1, and 3.2 unconditionally calledMountHelper.Unmount(mountedDirName). WhenDirectory.Deleteremoves the mount point directory as part of the delete operation, the subsequentUnmountcall throws (Win32 error 4390/3), gets caught by the scenario'scatch, and setss_pass = false.Changes
Delete_MountVolume.cs— Scenarios 1, 2, 3.1, 3.2finallyblocks: WrapUnmount/DeleteDircalls withDirectory.Exists(mountedDirName), matching the pattern already used in Scenarios 3.3–3.5.Original prompt
Problem
The
Directory_Delete_MountVolume.RunTest()test insrc/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/Directory/Delete_MountVolume.csfails because thefinallyblocks in Scenarios 1, 2, 3.1, and 3.2 unconditionally callMountHelper.Unmount(mountedDirName), butDirectory.Deletemay have already removed the mount point directory. WhenUnmounttries to callDeleteVolumeMountPointon a path that no longer exists, it throws (Win32 error 4390 or 3), which gets caught by the scenario'scatchblock and setss_pass = false, failing the test.Scenarios 3.3, 3.4, and 3.5 already correctly guard their cleanup with
if (Directory.Exists(mountedDirName))before attempting to unmount. The fix is to apply the same pattern to the 4finallyblocks that are missing it.Fix
Change the
finallyblocks in Scenarios 1, 2, 3.1, and 3.2 to guard theUnmountandDeleteDircalls withDirectory.Exists(mountedDirName), matching the existing pattern used in Scenarios 3.3–3.5.There are exactly 4
finallyblocks to change. Each one currently looks like:Each should be changed to:
The 4 locations in the file
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/Directory/Delete_MountVolume.csare:Do not change anything else. No changes to
s_pass,Eval, method signatures, test structure, or any other lines. Only add theDirectory.Existsguard to these 4finallyblocks.The following is the prior conversation context from the user's chat exploration (may be truncated):
User: explain this test to me and what s_pass asserts
Assistant: ## Explanation of
Delete_MountVolume.RunTest()This is a Windows-specific test that verifies
Directory.Deleteworks correctly when NTFS volume mount points are involved. A volume mount point is a directory that acts as an entry point to another volume (similar to a drive letter, but mapped to a folder).What It Tests
The test runs 7 scenarios, all variations of the same theme — deleting directories that are accessed through or contain mounted volumes:
The Pattern in Each Scenario
MountHelper.Mount.finallyblock.What
s_passAssertss_passis a boolean accumulator for the overall test result. It starts astrueand is set tofalsein two situations:Eval(...)call returnsfalse—Evalchecks a condition (e.g., "directory was deleted") and setss_pass = falseif the condition fails, logging the error.catchblock setss_pass = falseand logs the error.At the very end, in the outermost
finally:This is a single assertion that summarizes all scenarios. If any scenario failed a condition or threw an exception,
s_passwill befalseand the test fails. This pattern was common in older test code — it lets the test run all scenarios even if one fails (rather than stopping at the firstAssertfailure), collecting all errors before reporting.Why It's Disabled
The
[ActiveIssue("https://github.com/dotnet/runtime/issues/14378")]attribute means this...This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.