Wait for the session to end when dropping BackgroundSession

Since 0.16, dropping a BackgroundSession unmounted the filesystem but
left the session thread detached, so drop returned before the session
had ended and Filesystem::destroy could run much later - or never, when
the process exited first (#411). That also meant the destroy-exactly-
once guarantee did not effectively extend to BackgroundSession (#239).

Add a Drop impl that unmounts and then joins the session thread,
restoring the pre-0.16 blocking behavior: destroy has run by the time
drop returns. Two deliberate exceptions avoid blocking forever: a
session created via Session::from_fd has no mount to remove, so nothing
in drop can end it and its thread stays detached; and if the unmount
itself fails, the session is still running, so the thread is detached
as before instead of joined.

The guard field of BackgroundSession is now private; use join() or
umount_and_join(), which keep their existing signatures.

Fixes #411
Fixes #239

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TfPeUUeQKYjo26Y9mQG9sA

Author: claude

CHANGELOG.md

@@ -1,6 +1,13 @@
 # FUSE for Rust - Changelog
 
 ## Unreleased
+* Dropping a `BackgroundSession` now unmounts the filesystem and waits for the session to
+  end, guaranteeing that `Filesystem::destroy` has run when drop returns (#239, #411). This
+  restores the pre-0.16 blocking drop behavior. Sessions created via `Session::from_fd` have
+  no mount to remove and are still left detached on drop; use `join()` after ending them.
+  The `guard` field of `BackgroundSession` is now private - use `join()`/`umount_and_join()`
+* The pure-rust mount backend now reports `fusermount` unmount failures instead of silently
+  ignoring them, so a failed unmount cannot make `BackgroundSession` teardown wait forever
 * Treat `ECONNABORTED` from the FUSE device as a clean session end (#212): with
   `FUSE_ABORT_ERROR` negotiated, aborting the connection made `Session::run()` and
   `BackgroundSession::umount_and_join()` return an error instead of ending normally

src/mnt/fuse_pure.rs

@@ -84,7 +84,7 @@ impl MountImpl {
             if err == nix::errno::Errno::EPERM {
                 // Linux always returns EPERM for non-root users.  We have to let the
                 // library go through the setuid-root "fusermount -u" to unmount.
-                fuse_unmount_pure(&self.mountpoint);
+                fuse_unmount_pure(&self.mountpoint)?;
                 return Ok(());
             } else {
                 return Err(err.into());
@@ -120,21 +120,27 @@ fn fuse_mount_pure(
     fuse_mount_fusermount(mountpoint, options, acl)
 }
 
-fn fuse_unmount_pure(mountpoint: &CStr) {
+fn fuse_unmount_pure(mountpoint: &CStr) -> io::Result<()> {
     #[cfg(target_os = "linux")]
     {
         if nix::mount::umount2(mountpoint, nix::mount::MntFlags::MNT_DETACH).is_ok() {
-            return;
+            return Ok(());
         }
     }
     #[cfg(target_os = "macos")]
     {
         if nix::mount::unmount(mountpoint, nix::mount::MntFlags::MNT_FORCE).is_ok() {
-            return;
+            return Ok(());
         }
     }
+    fusermount_unmount(&detect_fusermount_bin(), mountpoint)
+}
 
-    let mut builder = Command::new(detect_fusermount_bin());
+/// Lazily unmount via the given fusermount binary. Helper failures are reported,
+/// so that callers do not mistake a still-mounted filesystem for an unmounted
+/// one - BackgroundSession would otherwise wait forever for the session to end.
+fn fusermount_unmount(fusermount_bin: &str, mountpoint: &CStr) -> io::Result<()> {
+    let mut builder = Command::new(fusermount_bin);
     builder.stdout(Stdio::piped()).stderr(Stdio::piped());
     builder
         .arg("-u")
@@ -143,10 +149,17 @@ fn fuse_unmount_pure(mountpoint: &CStr) {
         .arg("--")
         .arg(OsStr::new(&mountpoint.to_string_lossy().into_owned()));
 
-    if let Ok(output) = builder.output() {
-        debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout));
-        debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr));
+    let output = builder.output()?;
+    debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout));
+    debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr));
+    if !output.status.success() {
+        return Err(io::Error::other(format!(
+            "fusermount failed to unmount ({}): {}",
+            output.status,
+            String::from_utf8_lossy(&output.stderr).trim()
+        )));
     }
+    Ok(())
 }
 
 fn detect_fusermount_bin() -> String {
@@ -499,3 +512,22 @@ fn fuse_mount_sys(
 ) -> Result<Option<DevFuse>, Error> {
     Ok(None)
 }
+
+#[cfg(all(test, target_os = "linux"))]
+mod test {
+    use std::ffi::CString;
+
+    /// A fusermount helper that cannot run or exits nonzero must report an error:
+    /// the filesystem is then still mounted, and treating that as success would
+    /// make teardown wait forever for a session that never ends.
+    #[test]
+    fn fusermount_unmount_reports_failure() {
+        let mountpoint = CString::new("/nonexistent").unwrap();
+        // Helper exits nonzero (/bin/false ignores its arguments)
+        assert!(super::fusermount_unmount("/bin/false", &mountpoint).is_err());
+        // Helper cannot be executed at all
+        assert!(super::fusermount_unmount("/nonexistent/fusermount", &mountpoint).is_err());
+        // Helper reports success
+        assert!(super::fusermount_unmount("/bin/true", &mountpoint).is_ok());
+    }
+}

src/session.rs

@@ -227,7 +227,8 @@ impl<FS: Filesystem> Session<FS> {
     }
 
     /// Run the session loop in a background thread. If the returned handle is dropped,
-    /// the filesystem is unmounted and the given session ends.
+    /// the filesystem is unmounted, and this blocks until the session has ended and
+    /// `Filesystem::destroy` has run.
     pub fn spawn(self) -> io::Result<BackgroundSession> {
         let sender = self.ch.sender();
         // Take the fuse_session, so that we can unmount it
@@ -236,7 +237,7 @@ impl<FS: Filesystem> Session<FS> {
             .name("fuser-bg".to_string())
             .spawn(move || self.run())?;
         Ok(BackgroundSession {
-            guard,
+            guard: Some(guard),
             sender,
             mount,
         })
@@ -565,33 +566,57 @@ impl<FS: Filesystem> SessionEventLoop<FS> {
 }
 
 /// The background session data structure
+///
+/// Dropping this unmounts the filesystem and blocks until the session has ended,
+/// which guarantees that `Filesystem::destroy` has run when drop returns. Because
+/// of that, it must not be dropped from within a filesystem callback, which runs
+/// on the session thread being waited for. For a session created via
+/// `Session::from_fd` there is no mount to remove, so dropping cannot end the
+/// session and leaves its thread detached; end the session externally and use
+/// `join` to wait for it instead. If unmounting fails, the thread is likewise
+/// left detached rather than waiting for a session that cannot end.
 #[derive(Debug)]
 pub struct BackgroundSession {
-    /// Thread guard of the background session
-    pub guard: JoinHandle<io::Result<()>>,
+    /// Thread guard of the background session. None once joined.
+    guard: Option<JoinHandle<io::Result<()>>>,
     /// Object for creating Notifiers for client use
     sender: ChannelSender,
     /// Ensures the filesystem is unmounted when the session ends
     mount: Option<Mount>,
 }
 
 impl BackgroundSession {
-    /// Unmount the filesystem and join the background thread.
+    /// Unmount the filesystem and join the background thread, returning the
+    /// session result. `Filesystem::destroy` has run when this returns.
     pub fn umount_and_join(mut self) -> io::Result<()> {
         if let Some(mount) = self.mount.take() {
-            mount.umount()?;
+            if let Err(err) = mount.umount() {
+                // The filesystem is still mounted and the session still running,
+                // so joining would block indefinitely; leave the thread detached,
+                // as before
+                self.guard.take();
+                return Err(err);
+            }
         }
-        self.join()
+        self.join_impl()
     }
 
     /// Returns an object that can be used to send notifications to the kernel
     pub fn notifier(&self) -> Notifier {
         Notifier::new(self.sender.clone())
     }
 
-    /// Join the filesystem thread.
-    pub fn join(self) -> io::Result<()> {
-        self.guard
+    /// Join the filesystem thread without unmounting first: blocks until
+    /// something else ends the session, e.g. an external unmount.
+    pub fn join(mut self) -> io::Result<()> {
+        self.join_impl()
+    }
+
+    fn join_impl(&mut self) -> io::Result<()> {
+        let Some(guard) = self.guard.take() else {
+            return Ok(());
+        };
+        guard
             .join()
             .map_err(|_panic: Box<dyn std::any::Any + Send>| {
                 io::Error::new(
@@ -602,6 +627,29 @@ impl BackgroundSession {
     }
 }
 
+impl Drop for BackgroundSession {
+    fn drop(&mut self) {
+        // Unmount and wait for the session to end, so that Filesystem::destroy
+        // has run by the time drop returns (issues #239 and #411). Without the
+        // join, the process could exit before the detached session thread got
+        // around to calling destroy
+        let Some(mount) = self.mount.take() else {
+            // No mount to remove (Session::from_fd): nothing here can end the
+            // session, so joining could block forever; leave the thread detached
+            return;
+        };
+        if let Err(err) = mount.umount() {
+            // Still mounted, so the session is still running and joining would
+            // block indefinitely
+            warn!("Failed to unmount filesystem during drop: {err}");
+            return;
+        }
+        if let Err(err) = self.join_impl() {
+            warn!("Session ended with an error during drop: {err}");
+        }
+    }
+}
+
 // The abort test uses fusectl, which only exists on Linux; on other targets the
 // whole module would be dead code
 #[cfg(all(test, target_os = "linux"))]
@@ -675,6 +723,42 @@ mod test {
         ManuallyDrop::into_inner(tmp);
     }
 
+    /// Dropping a BackgroundSession must unmount the filesystem and wait for the
+    /// session to end, so that Filesystem::destroy has run when drop returns
+    /// (issues #239 and #411). Previously the session thread was detached, and
+    /// destroy raced (or lost to) process exit.
+    #[test]
+    fn drop_waits_for_destroy() {
+        struct DestroyFs {
+            destroyed: Arc<std::sync::atomic::AtomicBool>,
+        }
+        impl Filesystem for DestroyFs {
+            fn destroy(&mut self) {
+                // Give drop a chance to return early: without the join, drop
+                // consistently wins this race and the assertion below fails
+                std::thread::sleep(std::time::Duration::from_millis(200));
+                self.destroyed
+                    .store(true, std::sync::atomic::Ordering::SeqCst);
+            }
+        }
+
+        let tmp = ManuallyDrop::new(tempfile::tempdir().unwrap());
+        let destroyed = Arc::new(std::sync::atomic::AtomicBool::new(false));
+        let fs = DestroyFs {
+            destroyed: destroyed.clone(),
+        };
+        let bg = Session::new(fs, tmp.path(), &Config::default())
+            .unwrap()
+            .spawn()
+            .unwrap();
+        drop(bg);
+        assert!(
+            destroyed.load(std::sync::atomic::Ordering::SeqCst),
+            "destroy() must have been called by the time drop returns"
+        );
+        ManuallyDrop::into_inner(tmp);
+    }
+
     /// The fusectl abort file for the FUSE mount at `mountpoint`: the connection
     /// directory is named after the mount's anonymous device number.
     fn fusectl_abort_path(mountpoint: &Path) -> Option<std::path::PathBuf> {