Wait for the session to end when dropping BackgroundSession#709
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97200974c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| warn!("Failed to unmount filesystem during drop: {err}"); | ||
| return; | ||
| } | ||
| if let Err(err) = self.join_impl() { |
There was a problem hiding this comment.
When an unprivileged pure-Rust mount must fall back to fusermount, MountImpl::umount_impl reports success even if the helper cannot run or exits nonzero: fuse_unmount_pure ignores both the command error and output.status. In that failure case the FUSE connection remains live, so this unconditional join_impl() waits forever during an ordinary BackgroundSession drop. Propagate the helper failure or verify that the connection ended before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed in the latest push by propagating the failure at its source. fuse_unmount_pure now returns io::Result: the fusermount invocation checks both the spawn result and the exit status (fusermount_unmount, with a unit test covering nonzero-exit, unspawnable-binary, and success), and umount_impl propagates it. A failed helper therefore surfaces as Err from Mount::umount(), and the existing error path in Drop/umount_and_join detaches instead of joining - no indefinite wait.
Two boundary notes:
- A successful
fusermount -zon a busy filesystem is not a failure: the session ends when the last user releases it, and drop intentionally blocks until then (the pre-0.16 semantics these issues ask to restore). - The
libfuse2/libfuse3EPERM fallbacks go through libfuse'sfuse_unmount_compat22/fuse_session_unmount, which arevoidand cannot report helper failures. That exposure is unchanged and predates this PR -umount_and_join()has joined after those unverified paths since 0.17. Making those backends verifiable would require fuser to bypass libfuse's own unmount, which is out of scope here.
Generated by Claude Code
9720097 to
154d2df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 154d2df6be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| warn!("Failed to unmount filesystem during drop: {err}"); | ||
| return; | ||
| } | ||
| if let Err(err) = self.join_impl() { |
There was a problem hiding this comment.
When built with libfuse2 and run unprivileged, this join can block forever if the compatibility unmount fails. The pure-Rust fix addresses the prior case, but fresh evidence remains in src/mnt/fuse2.rs: lines 47-50 explicitly state that fuse_unmount_compat22 can leave the mount in place without reporting status, while lines 64-66 nevertheless return Ok(()); that success reaches this unconditional join while the session is still serving. Verify that the connection ended, or leave the thread detached for this backend, before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest push, and by upgrading rather than detaching: the libfuse2 EPERM path no longer uses fuse_unmount_compat22 at all. It now invokes fusermount directly through the same status-checked helper the pure-Rust backend uses (fusermount_unmount, shared in mnt), so a helper failure surfaces as Err and teardown detaches instead of joining. This is strictly better than skipping the join for that backend - the destroy-on-drop guarantee is kept whenever the unmount actually succeeded - and it drops compat22's other known hazard, the realpath that stats the mountpoint through the filesystem (the comment in this file already avoided fuse_unmount_compat22 for the primary path for exactly that reason). The now-unused extern declaration is removed.
libfuse3 remains the one backend whose EPERM fallback cannot report helper failures: its unmount is coupled to the fuse_session (which owns the fd and gets destroyed in the same call sequence), so bypassing fuse_session_unmount is not a safe local change. Its fuse_kern_unmount does at least skip dead connections itself, and the exposure is unchanged from what umount_and_join() has had since 0.17.
Generated by Claude Code
bddca28 to
a2ec561
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2ec561ed0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| warn!("Failed to unmount filesystem during drop: {err}"); | ||
| return; | ||
| } | ||
| if let Err(err) = self.join_impl() { |
There was a problem hiding this comment.
When the libfuse3 backend runs unprivileged and its fusermount helper cannot unmount, this join can still block forever. Fresh evidence beyond the fixed libfuse2 path remains in src/mnt/fuse3.rs: MountImpl::umount_impl calls the void fuse_session_unmount, destroys the libfuse session, and returns Ok(()) without knowing whether the mount was removed; the duplicated FUSE descriptor used by the background loop remains live if removal failed. Propagate or independently verify the libfuse3 unmount result before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest push, together with the auto-unmount case, by a single mechanism instead of more per-backend surgery: after a successful unmount request, Drop now waits (bounded, 5s) for the kernel connection to actually end - polling the FUSE device fd the session already holds - and joins only then; if the connection is still alive, it warns and detaches. A silently-failed fusermount3 leaves the connection alive, so this covers the libfuse3 fallback (and any other unmount path that cannot report failure) without bypassing fuse_session_unmount, whose coupling to the session made per-backend verification unsafe. The wait also tolerates asynchronous teardown (auto_unmount, kernel-side delays), and a slow Filesystem::destroy is unaffected: the bound applies only to the connection ending, while the join itself still waits for destroy to complete. umount_and_join() keeps its explicit, documented blocking semantics.
Generated by Claude Code
| warn!("Failed to unmount filesystem during drop: {err}"); | ||
| return; | ||
| } | ||
| if let Err(err) = self.join_impl() { |
There was a problem hiding this comment.
With the pure-Rust AutoUnmount path, this join can still wait forever if the long-lived fusermount helper dies after mounting or fails during its eventual cleanup. Fresh evidence beyond the regular-helper fix is src/mnt/fuse_pure.rs:78-82, where teardown merely closes auto_unmount_socket and returns Ok(()) without retaining the child handle, checking its status, or verifying that the connection ended. Do not join until that asynchronous unmount is confirmed, or detach the thread when it cannot be confirmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest push by the same bounded verification as the libfuse3 thread: after a successful unmount request, Drop waits up to 5s for the kernel connection to end (polling the session's own FUSE device fd) and joins only then; otherwise it warns and detaches. For the AutoUnmount path this handles both directions: the normal case, where fusermount unmounts asynchronously moments after the socket closes (the wait absorbs the latency and the join then proceeds), and the pathological one, where the fusermount process died without unmounting and the connection never ends (the wait expires and the thread is detached instead of blocking forever).
Generated by Claude Code
| teardown wait forever. The `libfuse2` backend no longer uses `fuse_unmount_compat22`, | ||
| which also used to stat the mountpoint through the filesystem |
There was a problem hiding this comment.
The sentence naming fuse_unmount_compat22 and describing its internal mountpoint stat is an implementation detail rather than user-facing impact. Keep the preceding failure-reporting behavior in the changelog and move or remove this backend-specific mechanism; the repository's AGENTS.md explicitly requires release notes to be brief and focused on user impact.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in the latest push: the release note keeps only the user-facing behavior (fusermount unmount failures are now reported) and the fuse_unmount_compat22 mechanism sentence is removed. The drop bullet was also reworded to describe when drop waits and when it detaches, without backend internals.
Generated by Claude Code
345a5e1 to
ee67787
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee67787626
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// 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. |
There was a problem hiding this comment.
Document the exceptions here rather than promising that every returned handle blocks until destroy runs. For Session::from_fd, BackgroundSession::drop returns immediately because mount is None, and after an unmount failure it also returns with the session thread detached; callers relying on this method-level contract can therefore free resources before Filesystem::destroy has run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in the latest push: spawn()'s doc no longer promises unconditional blocking - it now says the session is waited for "except in the cases documented on [BackgroundSession], where the session thread is detached instead", linking to the struct doc that enumerates all three exceptions (from_fd sessions, unmount failure, and a connection still alive after the bounded wait). Verified with cargo doc under --deny warnings for the intra-doc link.
Generated by Claude Code
ee67787 to
5ce0330
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ce0330825
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// (issues #239 and #411). Previously the session thread was detached, and | ||
| /// destroy raced (or lost to) process exit. |
There was a problem hiding this comment.
The references to issue numbers and the behavior before this patch describe change history rather than the lasting invariant being tested. Keep the preceding statement that drop waits for Filesystem::destroy, but remove these commit-specific details to comply with the repository's comment style guide.
AGENTS.md reference: AGENTS.md:L6-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in the latest push: the test doc keeps only the invariant (drop unmounts and waits so that Filesystem::destroy has run when it returns); the issue references and the pre-change behavior sentence are removed.
Generated by Claude Code
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
5ce0330 to
e91c9e6
Compare
2 participants
Fixes #411
Fixes #239
Problem
Since 0.16, dropping a
BackgroundSessionunmounts the filesystem but leaves thefuser-bgthread detached: drop returns before the session has ended, soFilesystem::destroyruns much later - or never, when the process exits first. #411 reports exactly this regression from the pre-0.16 blocking drop (worker threads that rely ondestroyfor cleanup hang), and it is why the destroy-exactly-once guarantee did not effectively extend toBackgroundSession(#239): the session structure that runsdestroyon its way out was detached from the caller's lifetime.Fix
BackgroundSessionnow has aDropimpl that unmounts and then joins the session thread.run()already callsdestroy()after all event loop threads have joined (andFilesystemHolderbackstops early-exit paths), so the join is sufficient to guaranteedestroyhas run when drop returns - restoring the pre-0.16 behavior, on the thread that was already responsible for it.Two deliberate exceptions avoid blocking forever, both documented on the struct:
Session::from_fdhas no mount to remove, so nothing in drop can end the session; its thread stays detached (this is the_mount: Nonecase identified in the BackgroundSession::drop() no longer waits for filesystem destroy, causing cleanup issues #411 discussion), and callers should end the session themselves and usejoin();umount_and_join()keeps the same early-return contract).API notes:
join()andumount_and_join()keep their signatures; theguardfield is now private (required for aDropimpl to coexist with the consuming methods), which is a breaking change for code that accessed it directly. Dropping from within a filesystem callback would self-deadlock and is called out in the docs - the same was already true of callingumount_and_jointhere.Testing
New regression test
drop_waits_for_destroy: spawns a background session whosedestroysleeps 200ms before setting a flag, drops theBackgroundSession, and asserts the flag is set when drop returns. The sleep makes the race deterministic in both directions: without the fix drop consistently returns first and the test fails; with it, the join waits throughdestroy. Verified fail-before/pass-after on all three mount backends.cargo test --lib,cargo fmt --check, andcargo clippy --all-targetswithRUSTFLAGS=--deny warningspass for default features,libfuse2, andlibfuse3;cargo check --target x86_64-apple-darwin --features=macos-no-mount --all-targetspasses for the macOS configuration.🤖 Generated with Claude Code
https://claude.ai/code/session_01TfPeUUeQKYjo26Y9mQG9sA
Generated by Claude Code