Skip to content

Fix: Release the handles Files holds on a drive before ejecting it - #18906

Closed
Floral-Overcast wants to merge 2 commits into
files-community:mainfrom
Floral-Overcast:fix/eject-release-handles
Closed

Fix: Release the handles Files holds on a drive before ejecting it#18906
Floral-Overcast wants to merge 2 commits into
files-community:mainfrom
Floral-Overcast:fix/eject-release-handles

Conversation

@Floral-Overcast

Copy link
Copy Markdown

Resolved / Related Issues

Note: the issue isn't marked Ready to build, but it's a confirmed, member-acknowledged bug ("I will keep this open to track Files blocking eject issue"), so I hope a concrete fix is useful. Happy to adjust the approach if the team prefers.

Description

Ejecting a removable drive from Files frequently fails with Windows' "device is currently in use" dialog while ejecting the same drive from File Explorer works. The reason is that Files itself holds open handles on the volume and vetoes its own eject:

  • every pane showing the drive keeps a ReadDirectoryChangesW directory handle open (ShellViewModel.WatchForDirectoryChanges),
  • every expanded sidebar node on the drive owns a FileSystemWatcher,
  • RecycleBinWatcher arms a watcher on <drive>\$RECYCLE.BIN\<sid> for every non-network drive at startup and never releases it per-drive.

DriveHelpers.EjectDeviceAsync invoked the shell "eject" verb without releasing any of these, so the PnP query-remove failed. (Explorer avoids this by releasing its handles on DBT_DEVICEQUERYREMOVE.)

This PR makes EjectDeviceAsync release Files' own holds before invoking the verb:

  1. For every pane whose current folder is on the drive: close its directory watcher and navigate it Home.
  2. Stop sidebar subtree watchers rooted on the drive (StopWatchingSubfoldersAndDescendants).
  3. Drop the Recycle Bin watcher for that drive (new per-drive RecycleBinWatcher.StopWatcher(driveRoot) overload).
  4. Give handles a moment to close, then invoke the existing shell "eject" verb unchanged.

Not covered here (possible follow-ups): open preview-pane streams, in-flight size/search enumerations, and listening for DBT_DEVICEQUERYREMOVE so ejects initiated outside Files also succeed — that last one is the structurally complete fix but a much larger change.

Steps used to test these changes

  1. Built (x64 Release, sideload package) and installed on Windows 11.
  2. Inserted a microSD card, opened tabs on it, and browsed folders on the card.
  3. Pressed Eject in Files: panes navigated Home and the card ejected successfully on the first attempt. Previously the same flow reliably failed with "device is currently in use" and required closing Files or ejecting from Explorer.

🤖 Generated with Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Floral-Overcast Floral-Overcast changed the title Fix: Release Files's own drive handles before ejecting a removable drive Fix: Release the handles Files holds on a drive before ejecting it Aug 30, 2026
@CLAassistant

CLAassistant commented Aug 30, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Josh65-2201

Copy link
Copy Markdown
Member

The linked issue is not ready to be worked on, please read the contribution guidelines, ready to work on issues are at Files task board (view)

@hez2010 hez2010 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@codex review apart from my comments.

Ioc.Default.GetRequiredService<IStorageTrashBinService>().Watcher.StopWatcher(driveRoot);

// Give the released handles a moment to close before the shell issues the removal query
await Task.Delay(300);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems weird to have a Delay inside ReleaseDriveHandlesAsync. Can you move this delay to EjectDeviceAsync?

@hez2010 hez2010 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The per-drive recycle-bin watcher is disposed and removed before the eject verb runs, but nothing recreates it. RecycleBinWatcher.StartWatcher() is only called by the watcher's constructor, and the app keeps the same singleton watcher. Consequently, this breaks recycle-bin notifications for that volume both when eject fails and when a successfully ejected volume is later reinserted, until Files restarts. Please add a lifecycle that re-arms monitoring when the volume remains/returns (and ensure it does not duplicate the still-active watchers for other volumes), or release this handle in a way that is paired with device removal/addition notifications.

hez2010

This comment was marked as outdated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Floral-Overcast

Copy link
Copy Markdown
Author

Thanks for the review @hez2010 — both points addressed in de8e22a:

  • Recycle Bin watcher lifecycle: RecycleBinWatcher now has an idempotent per-drive StartWatcher(driveRoot) (skips network drives, missing $RECYCLE.BIN\<sid>, and drives that already have an active watcher; the startup StartWatcher() loops over it). It is re-armed in two places:

    1. EjectDeviceAsync waits after invoking the verb and, if the volume is still mounted (eject vetoed), restarts the watcher for that drive.
    2. DrivesViewModel.Watcher_DeviceAdded starts a watcher for any drive that appears, which covers reinsertion of an ejected drive — and also fixes the pre-existing gap where drives inserted after app startup never got a Recycle Bin watcher at all (previously watchers were only created once in the constructor).

    The dedupe check means neither path can double-arm a volume that's already watched.

  • Delay placement: moved out of the release helper into EjectDeviceAsync; the helper is now synchronous (ReleaseDriveHandles).

🤖 Generated with Claude Code

@yair100 yair100 closed this Sep 1, 2026
@yair100

yair100 commented Sep 1, 2026

Copy link
Copy Markdown
Member

This PR is generated by AI, closing.

@Floral-Overcast

Floral-Overcast commented Sep 1, 2026

Copy link
Copy Markdown
Author

Hi, I'm a professional UX Designer attempting to implement this software into my workflow, just trying to add some basic functions I found missing in my own use of this software. I don't have a particular ownership or stake in whether or not this change is public, and I respect the process, but I was unaware I was breaking rules. Could you clarify what rules I broke or just where I could find them?

@hez2010

hez2010 commented Sep 1, 2026

Copy link
Copy Markdown
Member

I was unaware I was breaking rules. Could you clarify what rules I broke or just where I could find them?

You're not breaking any rules. For AI-generated PRs, we generally prefer contributions from people who are already familiar with the existing codebase, as that helps reduce the review burden. AI-generated PRs tend to contain subtle issues that can be difficult to identify and often require several rounds of changes before reaching a clean result.

For example, the correct way of fixing the linked issue is to subscribe CM_Register_Notification and release the handles in the callback, so that it can handle device ejection from explorer as well and doesn't have to require doing so in Files.

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.

Bug: Can't eject a hard drive as long as the Files app is running

5 participants