Rewrite GUI with tabbed UI, fix broken build, de-panic mount setup - #3
Merged
Conversation
The GUI binary did not compile at HEAD (E0382 borrow-after-move in start_mount) because CI never built the gui feature. This change fixes the build, restructures the GUI, and hardens the library setup path. GUI (src/bin/hf-mount-gui/, replaces the 2,637-line single file): - Real tabbed layout (Mount / Activity / Setup) instead of a decorative sidebar; bottom status bar with state chip, detail, and mounted elapsed time; scrollable form so short windows no longer clip. - Stability: background-worker status is now read by a dedicated poller thread (2s interval) instead of spawning tasklist.exe and statting a possibly-wedged NFS drive on every UI frame; unmount runs on a worker thread; foreground mounts stop through a cooperative shutdown handle that also works while the mount command is still retrying. - New features: free drive-letter picker on Windows (GetLogicalDrives, no probing), token show/hide, recent-sources dropdown, inline source validation, copy session log, per-check fix actions in Setup. Library: - setup::build/build_with_runtime return Result instead of panicking; new Error::Setup variant; the FUSE sidecar no longer needs catch_unwind to report setup failures. - nfs::mount_nfs_with_callback takes NfsMountParams with an optional MountShutdown for cooperative stop; Linux/macOS mount commands use tokio::process; the mount-disappeared probe runs via spawn_blocking; the Windows retry loop no longer ends in unreachable!(). - New hf_mount::windows module shares drive-letter/System32 helpers that were duplicated across nfs.rs, setup.rs, and the GUI; the pure parsing logic now has tests that run on Linux CI. CI: clippy + unit tests for the nfs,gui feature combo on the Linux runner, so GUI breakage is caught on every PR. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughModularizes the GUI into many files, adds background-worker status-file IPC and worker polling, centralizes platform integration and preflight/autostart, implements theme/widgets/tabs, and converts setup/mount/staging flows from panic-based code to Result-returning constructors with cooperative NFS shutdown. ChangesGUI Refactor & Setup Improvements
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Lint & Unit Tests failed in one second on this fork: the job targets the huggingface-internal runner group (hf-mount-ci-pub), which forks cannot use, so the main CI never actually ran here — which is also how a non-compiling GUI landed on the default branch unnoticed. Run lint-test on ubuntu-latest (works upstream too) and gate the internal-registry setup plus the five integration jobs (smoke, fsx, xfstests, pjdfstest, bench) to github.repository == 'huggingface/hf-mount' so they skip cleanly on forks instead of queuing forever on unavailable runners. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
![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: 1de4f13515
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Stop now terminates a background worker that has not mounted yet (taskkill on Windows, SIGTERM on Unix) instead of running a doomed unmount while the worker carries on and mounts anyway; the status file is overwritten so later launches don't see a stale Mounting claim, and a belt unmount cleans a mount that raced the kill. - A stale terminal worker status file no longer re-clobbers newer local status every poll: terminal reports are mirrored only when their content changes or at startup. - Cancellation is reported as Stopped, not Failed: the Windows retry loop returns a clean-stop marker instead of an Interrupted error, and the GUI maps post-stop errors to Stopped. - macOS/Linux mount commands race against the shutdown handle with kill_on_drop, so Stop interrupts a hung mount.nfs instead of waiting it out. - WorkerPoller shutdown joins with a 250ms bound and detaches if the thread is blocked probing a wedged mount, so window close cannot hang. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ac08ee02f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- CI: gate internal-runner jobs and the registry setup on the PR *head* repository, not github.repository (which is the base repo on fork PRs and would have run untrusted fork code on internal runners). - Unix worker termination signals the process group, so a mount.nfs helper spawned before the worker installs its SIGTERM handler dies with it instead of completing the mount post-kill. - A failed worker spawn clears the provisional Mounting status file so the poller doesn't report a phantom live worker for the staleness window. - unmount_nfs reports success; shutdown events now say when the unmount failed and the target needs manual cleanup instead of claiming a clean stop. - The mount-liveness probe is its own select arm, so a probe wedged on a dead mount no longer blocks the shutdown/signal/UMNT branches. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf9aa1869
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ntime teardown - mount_point_appears_active now consults the platform mount table / filesystem type (shared nfs::is_mounted, newly public) instead of Path::exists, so a leftover directory from a crashed worker or a reboot no longer reads as a live mount that pins the GUI. - Worker PIDs read back from the status file are verified to still identify as an hf-mount worker (cmdline marker on Linux/macOS, image name on Windows) before being trusted for liveness or termination — a recycled PID can no longer get an unrelated process killed. Stale records are cleared instead. - The owned tokio runtime tears down via shutdown_timeout(5s) through an OwnedRuntime guard, so a blocking probe wedged on a dead mount cannot stall Stop or window close at runtime-drop time. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 23-29: Add persist-credentials: false to the actions/checkout step
so the workflow token is not written into local git config for the fork-exposed
job; locate the checkout step using the
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd reference and add the
persist-credentials: false property under that step to explicitly disable Git
credential persistence.
In `@src/bin/hf-mount-gui/app.rs`:
- Around line 829-833: The early return in on_exit() when self.active_background
is true can abort a running background stop started by stop_mount(); modify
on_exit() to detect an in-flight stop by inspecting self.stop_thread (or the
equivalent join/handle stored by stop_mount()) and wait for it to finish (or
join it) before returning, using a bounded timeout/grace period to avoid
blocking forever; ensure you clear or take the handle (e.g. take() the Option)
so the join only happens once and preserve the existing active_background logic
so foreground behavior is unchanged.
In `@src/bin/hf-mount-gui/platform.rs`:
- Around line 204-205: The current Windows check uses
stdout.contains(&format!(",\"{pid}\",")) which can match the PID string in other
columns; update process_is_running to parse the CSV output explicitly: read
stdout line-by-line as CSV (e.g., csv::ReaderBuilder::from_reader or a simple
CSV split that respects quotes), extract the PID column (column index 1 in
tasklist CSV rows) and compare that field exactly to pid.to_string() instead of
using stdout.contains; replace the stdout.contains call and refer to the pid
variable and process_is_running function when making the change.
In `@src/bin/hf-mount-gui/profile.rs`:
- Around line 108-110: The validator currently only checks trimmed.contains('/')
which still allows values like "a/b/c"; update the inline check for source ==
GuiSource::Bucket to split trimmed by '/' and ensure there are exactly two
non-empty segments (e.g. parts.len() == 2 && !parts[0].is_empty() &&
!parts[1].is_empty()); if the check fails return the same user-facing message
("Buckets are namespace/bucket, e.g. myuser/my-bucket.") so invalid bucket IDs
with zero, more than one, or empty segments are rejected early.
In `@src/bin/hf-mount-gui/util.rs`:
- Around line 95-103: The Windows branch in write_file_replace currently calls
std::fs::remove_file(path) before std::fs::rename, which can permanently lose
the original on a rename failure; remove the pre-delete and let std::fs::rename
perform the replace (or, if you need stricter Windows semantics, replace the
rename path with a Windows single-step replace primitive such as ReplaceFileW),
i.e., eliminate the remove_file(path) call and ensure error handling still
cleans up temp_path on failure while reporting the original and temp paths in
the error message.
In `@src/bin/hf-mount-gui/worker.rs`:
- Around line 235-247: Wrap the early startup sequence (calls to
load_mount_profile, profile_mount_source, profile_mount_options) so any Err
triggers a worker-owned status update that overwrites the provisional Mounting
record: catch the error, call write_worker_status with WorkerState::Failed (or
appropriate error state), a short message like "Background worker failed to
start", the error string (format!("{}", err)) as details, include
Some(std::process::id()) as the owner and the best-known mount point (or None if
unavailable), then return the original error; use the existing functions
append_worker_log and write_worker_status and the existing symbols
load_mount_profile, profile_mount_source, profile_mount_options to locate where
to add the error-handling/status-write logic.
- Around line 66-84: The read_worker_status() loop currently retries parse
failures but returns immediately on std::fs::read() errors; change it to treat
transient read errors (at least std::io::ErrorKind::NotFound and
PermissionDenied which happen during write_file_replace() on Windows) as
retryable: if std::fs::read(&path) returns an Err and attempt < 2, sleep briefly
and continue the loop, otherwise return the formatted error as before; keep
references to worker_status_path(), path, and read_worker_status() to locate the
change.
In `@src/nfs.rs`:
- Around line 926-941: The select branches call unmount_for_shutdown(...)
directly (and its fallback runs a blocking Command::status()), which can wedge
the task and prevent subsequent cleanup (server_handle.abort(),
portmapper_handle.abort(), vfs_for_shutdown.shutdown()); move the unmount work
off the select path by spawning it into a blocking task
(tokio::task::spawn_blocking or spawn) and await it with a timeout
(tokio::time::timeout) so the select branch can immediately proceed to break and
perform abort/shutdown even if unmount hangs; update the branches that call
unmount_for_shutdown and the fallback Command::status path to kick off the
offloaded unmount task and handle its result asynchronously (log
success/failure/timeout) without blocking the main shutdown flow.
In `@src/setup.rs`:
- Around line 339-343: build() currently calls build_runtime() which can panic
(via expect) and therefore bypass the Result error path; change build_runtime()
to return Result<tokio::runtime::Runtime, SetupError> (or appropriate error
type) instead of panicking, update its callers (including build_with_runtime if
needed) to accept a Runtime by value, and in build() call the fallible
build_runtime(), propagate any error as Err(...) instead of letting it abort,
then proceed to call build_with_runtime(source, options, is_nfs,
runtime.handle().clone()), set setup._owned_runtime = Some(runtime) and return
Ok(setup); ensure the unique symbols mentioned (build(), build_runtime(),
build_with_runtime(), MountSetup, _owned_runtime) are updated to match the new
fallible contract.
In `@src/xet.rs`:
- Around line 209-214: The code in StagingRoot::new currently uses
std::fs::create_dir_all which silently succeeds if a staging-* path already
exists, allowing collisions; change to attempt exclusive creation (use
std::fs::create_dir) and retry with a new random name on EEXIST until success
(or give up after a bounded number of attempts and return an Error).
Specifically modify the pub fn new(cache_dir: &Path, max_bytes: u64) ->
crate::error::Result<Self> to loop generating
cache_dir.join(format!("staging-{:016x}", rand_u64())) and call
std::fs::create_dir(&dir) (not create_dir_all), treating
std::io::ErrorKind::AlreadyExists as a retry case and other errors as failures;
ensure the final processLogger/Error message includes the dir and underlying
error and keep StagingRoot::drop semantics intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0173569c-be5f-431b-896d-46f5f6988aa0
📒 Files selected for processing (26)
.github/workflows/ci.ymlCargo.tomlREADME.mdsrc/bin/hf-mount-fuse-sidecar.rssrc/bin/hf-mount-gui.rssrc/bin/hf-mount-gui/activity_tab.rssrc/bin/hf-mount-gui/app.rssrc/bin/hf-mount-gui/autostart.rssrc/bin/hf-mount-gui/main.rssrc/bin/hf-mount-gui/mount_tab.rssrc/bin/hf-mount-gui/platform.rssrc/bin/hf-mount-gui/preflight.rssrc/bin/hf-mount-gui/profile.rssrc/bin/hf-mount-gui/setup_tab.rssrc/bin/hf-mount-gui/theme.rssrc/bin/hf-mount-gui/util.rssrc/bin/hf-mount-gui/widgets.rssrc/bin/hf-mount-gui/worker.rssrc/daemon.rssrc/error.rssrc/lib.rssrc/nfs.rssrc/setup.rssrc/test_mocks.rssrc/windows.rssrc/xet.rs
💤 Files with no reviewable changes (1)
- src/bin/hf-mount-gui.rs
…utdown - write_file_replace no longer pre-deletes the destination on Windows; std::fs::rename replaces atomically (MoveFileExW), closing a crash window that could lose the old file. - read_worker_status retries transient NotFound/PermissionDenied reads that can surface mid-replace on Windows, not just parse errors. - Background worker writes a Failed status if profile/source/options loading fails, overwriting the GUI's provisional Mounting claim instead of leaving it live for the staleness window. - Windows worker liveness parses the tasklist PID column explicitly rather than substring-matching the row. - Bucket IDs must be exactly namespace/bucket (reject extra/empty segments) at validation time, not deferred to mount setup. - on_exit waits (bounded) for an in-flight background stop thread so closing the window can't abort an unmount and orphan the mount. - Shutdown-time unmounts run on the blocking pool and are awaited with a 10s timeout, so a wedged umount can't delay server/portmapper/VFS teardown. - build_runtime returns Result; build() surfaces runtime-creation failure as Error::Setup instead of panicking. - StagingDir::new creates its dir exclusively and retries on collision, so a random-name clash can't share one staging root between mounts. - CI: persist-credentials: false on the fork-exposed checkout. https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a432f7f4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nfs.rs (1)
1247-1252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLinux
is_mountedcan report false negatives and trigger premature shutdown.At Line 1251, comparing
pathdirectly to/proc/mountsfield 2 is brittle (\040escaping, normalization differences like trailing slash). That can make an active mount look “disappeared,” causing the shutdown path to fire incorrectly.🔧 Suggested fix
#[cfg(target_os = "linux")] { + let wanted = std::path::Path::new(path) + .canonicalize() + .unwrap_or_else(|_| std::path::PathBuf::from(path)); std::fs::read_to_string("/proc/mounts") - .map(|s| s.lines().any(|line| line.split_whitespace().nth(1) == Some(path))) + .map(|s| { + s.lines().any(|line| { + line.split_whitespace() + .nth(1) + .map(proc_mounts_unescape) + .map(std::path::PathBuf::from) + .and_then(|p| p.canonicalize().ok().or(Some(p))) + .is_some_and(|p| p == wanted) + }) + }) .unwrap_or(false) } + +#[cfg(target_os = "linux")] +fn proc_mounts_unescape(raw: &str) -> String { + raw.replace("\\040", " ") + .replace("\\011", "\t") + .replace("\\012", "\n") + .replace("\\134", "\\") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nfs.rs` around lines 1247 - 1252, The is_mounted function currently compares the raw mountpoint token from /proc/mounts to the input path, which yields false negatives due to octal-escaped spaces (e.g. \040) and normalization differences; fix is_mounted by normalizing both sides before comparison: parse the mount lines as now, unescape octal sequences in the mountpoint token, normalize/remove trailing slashes and then compare using Path semantics (preferably try std::fs::canonicalize on both the provided path and the unescaped mountpoint and fall back to a normalized string comparison if canonicalize fails). Ensure you update the is_mounted implementation to perform unescaping, path normalization, and canonicalization fallback so mounts with escaped spaces or differing trailing slashes are detected correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/hf-mount-gui/app.rs`:
- Around line 851-862: The code currently calls handle.join() unconditionally
which can block indefinitely; instead, after the bounded wait loop in the branch
that checks self.active_background and takes self.stop_thread, only call
handle.join() if handle.is_finished() (or the deadline was reached and the
thread has finished); otherwise drop the JoinHandle to detach the thread so
window close remains bounded. Update the block around self.stop_thread.take(),
the while loop that checks handle.is_finished(), and the join call so join is
conditional (e.g., if handle.is_finished() { let _ = handle.join(); } ) and do
not block if platform::unmount_path is wedged.
In `@src/bin/hf-mount-gui/platform.rs`:
- Around line 203-215: In worker_process_alive (function worker_process_alive in
src/bin/hf-mount-gui/platform.rs) the marker is checked with contains(marker)
which can match substrings inside other arguments; change the check to match the
marker as a full command-line argument: after getting the ps output, convert
stdout to a String, split it into arguments (e.g., whitespace-split or use a
shell-words parser if you need quoted args), and return true only if any token
== marker; keep the rest of the existing early-return/error handling intact.
---
Outside diff comments:
In `@src/nfs.rs`:
- Around line 1247-1252: The is_mounted function currently compares the raw
mountpoint token from /proc/mounts to the input path, which yields false
negatives due to octal-escaped spaces (e.g. \040) and normalization differences;
fix is_mounted by normalizing both sides before comparison: parse the mount
lines as now, unescape octal sequences in the mountpoint token, normalize/remove
trailing slashes and then compare using Path semantics (preferably try
std::fs::canonicalize on both the provided path and the unescaped mountpoint and
fall back to a normalized string comparison if canonicalize fails). Ensure you
update the is_mounted implementation to perform unescaping, path normalization,
and canonicalization fallback so mounts with escaped spaces or differing
trailing slashes are detected correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 254c0424-ed08-462d-b573-cf67de4dd372
📒 Files selected for processing (10)
.github/workflows/ci.ymlsrc/bin/hf-mount-fuse-sidecar.rssrc/bin/hf-mount-gui/app.rssrc/bin/hf-mount-gui/platform.rssrc/bin/hf-mount-gui/profile.rssrc/bin/hf-mount-gui/util.rssrc/bin/hf-mount-gui/worker.rssrc/nfs.rssrc/setup.rssrc/xet.rs
…hutdown - profile: restore bucket subfolder support (regression). Validation now requires >=2 non-empty segments instead of exactly 2, so namespace/bucket/path mounts that Source::Bucket supports are accepted again; still rejects empty interior segments. - platform (Windows): validate the --background-worker marker via the process command line (PowerShell CIM), so a foreground GUI window that inherited a recycled PID is no longer mistaken for — or killed as — a background worker. Fails closed; Mounted liveness comes from the mount table so this only gates the brief Mounting/Stopping window. - platform (macOS): match the worker marker as a whole ps argument, not a substring. - worker: a recorded-but-dead/mismatched PID is now treated as stale immediately; the heartbeat-freshness fallback applies only to the provisional pid-less launch status. Mounted is confirmed via the mount table. - app: reconcile an existing background worker synchronously at startup so a second GUI gates Start immediately instead of racing the poller and spawning a duplicate worker; on_exit waits (bounded) for any in-flight stop/cleanup thread and only joins if finished; post-termination cleanup unmount is tracked in stop_thread rather than detached. - nfs: Linux is_mounted unescapes /proc/mounts octal escapes and normalizes trailing slashes so an active mount isn't misread as disappeared (string-only; no canonicalize that could block). https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c7873d165
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/bin/hf-mount-gui/platform.rs (1)
138-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn real launcher failures from
open_mount_point.This path uses
spawn(), so any successfully startedxdg-open/open/explorer.exeprocess is reported as success even when it immediately exits non-zero for a missing or invalid target.src/bin/hf-mount-gui/app.rslogs "Opened mount point" onOk(()), so the GUI currently reports success for failed open attempts.Suggested fix
pub fn open_mount_point(mount_point: Option<&Path>) -> Result<(), String> { let mount_point = mount_point.ok_or_else(|| "No active mount point is recorded.".to_string())?; let target = open_target(mount_point)?; + if !Path::new(&target).exists() { + return Err(format!("Mount point does not exist: {target}")); + } - open_command(&target) - .spawn() - .map_err(|e| format!("Failed to open mount point: {e}"))?; + let status = open_command(&target) + .status() + .map_err(|e| format!("Failed to open mount point: {e}"))?; + if !status.success() { + return Err(format!("Open command exited with {status}")); + } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/hf-mount-gui/platform.rs` around lines 138 - 145, The current open_mount_point uses open_command(&target).spawn() which returns success as soon as the launcher process starts; instead run the command to completion and return its exit status so real launcher failures propagate. Replace the spawn() call with calling .output() (or .status()) on the Command returned by open_command(&target), map any Io error into the same Err path, then check the returned ExitStatus: if status.success() return Ok(()), otherwise return Err with a descriptive message including the exit code and (if using .output()) stderr/stdout text; keep references to open_mount_point, open_target and open_command so the change is applied in that function.src/bin/hf-mount-gui/app.rs (1)
916-921:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep the forced-unmount fallback off the shutdown thread.
Lines 455-456 already treat
platform::unmount_pathas potentially blocking on a wedged NFS mount, buton_exit()calls it synchronously here after the 8-second grace period. If that unmount wedges, window close still hangs indefinitely, so this path breaks the bounded-shutdown behavior this method is trying to preserve. Please move the fallback behind a timeout-aware platform helper or a detached cleanup mechanism that cannot blockon_exit().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/hf-mount-gui/app.rs` around lines 916 - 921, The forced unmount call in on_exit() that directly calls platform::unmount_path(mount_point) can block shutdown; change this to perform the fallback unmount off the shutdown thread by submitting it to a detached worker or a timeout-aware helper: either spawn a detached thread (std::thread::spawn) or use tokio::spawn_blocking to call platform::unmount_path(mount_point) and return immediately, or wrap the blocking call in a helper like platform::unmount_path_with_timeout(mount_point, Duration) that runs the blocking unmount in a separate thread and enforces a timeout; update the code that checks handle.is_finished() / self.active_mount_point in on_exit() to call this nonblocking helper instead of calling platform::unmount_path directly so on_exit() cannot be blocked by a wedged NFS unmount.src/nfs.rs (2)
840-884:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report
MountedwhenHF_MOUNT_SKIP_AUTO_MOUNTskippedmount.exe.Line 840 intentionally leaves the client mount undone, but Line 876 still emits
NfsMountEvent::Mountedand Line 882 still signals readiness. That reports a live mount before one exists, so the GUI/daemon can transition to a mounted/ready state against an unmounted path.Suggested minimal fix
- info!("NFS mount active at {}", mount_point_str); - on_event(NfsMountEvent::Mounted { - mount_point: mount_point_str.to_string(), - }); - - // Signal the parent process that the mount is live (daemon mode). - if let Some(guard) = daemon_guard { - guard.notify_ready(); - } + if !skip_auto_mount { + info!("NFS mount active at {}", mount_point_str); + on_event(NfsMountEvent::Mounted { + mount_point: mount_point_str.to_string(), + }); + + // Signal the parent process that the mount is live (daemon mode). + if let Some(guard) = daemon_guard { + guard.notify_ready(); + } + } else { + info!("NFS server is ready; waiting for a manual mount of {}", mount_point_str); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nfs.rs` around lines 840 - 884, The code currently logs NfsMountEvent::Mounted and calls daemon_guard.notify_ready() unconditionally even when skip_auto_mount is true (the branch that skips invoking mount.exe), which reports a live mount that doesn't exist; modify the flow so those two actions only happen when a real mount was performed—e.g. introduce a boolean like mount_performed (set true after successful mount in the else branch where mount_windows_nfs_with_retry returns Some and output.status.success()), and wrap the calls to on_event(NfsMountEvent::Mounted { mount_point: mount_point_str.to_string() }) and daemon_guard.notify_ready() in a conditional that checks mount_performed (or alternatively return early from the skip_auto_mount branch instead of falling through).
757-766:⚠️ Potential issue | 🟠 MajorDon’t let shutdown win the mount-vs-cancel race (avoid skipping unmount)
Thesetokio::select!shutdown arms return early (abort server +vfs_for_shutdown.shutdown()) before reaching the later shutdown/unmount path, so ifmount_nfs/mount.nfs/mount.exehas already completed successfully in the same poll, the server/portmapper teardown can happen while the client mount remains.
- macOS (
src/nfs.rs:757-766):wait_for_shutdowncan win overcommand.status(), returningOk(())without schedulingspawn_unmount(...).- Linux (
src/nfs.rs:795-804): same forcommand.output(), returningOk(())withoutspawn_unmount(...).- Windows retry (
src/nfs.rs:1354-1356, and the caller handlingOk(None)):wait_for_shutdowncan win overcommand.output(), causingOk(None)→ caller returnsOk(())without unmount.Suggested direction
- let status = tokio::select! { + let status = tokio::select! { + biased; status = command.status() => status?, _ = wait_for_shutdown(shutdown.as_ref()) => { server_handle.abort(); on_event(NfsMountEvent::ShuttingDown { reason: "stop requested".to_string(), @@ - let output = tokio::select! { + let output = tokio::select! { + biased; output = command.output() => output?, _ = wait_for_shutdown(shutdown.as_ref()) => return Ok(None), };(Apply the same completion-first behavior to the Windows retry helper as well, so a concurrent successful
mount.exeresult can’t be treated as cancelled.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nfs.rs` around lines 757 - 766, Change the concurrent-selects so a concurrently-ready mount command always wins and thus the later unmount/spawn_unmount path runs: in src/nfs.rs at 757-766 (macOS) and 795-804 (Linux) add "biased;" to the tokio::select! and ensure the command arm (status = command.status() / output = command.output()) is the first arm so it will be picked when both are ready (leaving the existing server_handle.abort(), on_event(NfsMountEvent::ShuttingDown { .. }), and vfs_for_shutdown.shutdown() logic in the shutdown arm unchanged); do the same change for the Windows retry helper at src/nfs.rs 1354-1356 (add "biased;" and make the command.output() arm the first arm) so a simultaneous successful mount.exe result isn’t treated as cancelled and spawn_unmount(...) still runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/hf-mount-gui/worker.rs`:
- Around line 153-172: worker_status_is_live currently treats any pid-less
status as live based solely on the heartbeat; update worker_status_is_live to
treat terminal states as dead by checking WorkerStatus.state before using the
heartbeat fallback: if status.pid is None and status.state is
WorkerState::Stopped or WorkerState::Failed then return false (these are written
by mark_worker_stopped() and early startup failure paths), otherwise keep the
existing heartbeat logic that uses updated_at_secs and
WORKER_STATUS_STALE_AFTER_SECS to decide liveness; keep existing
mounted/Platform mount check and worker_process_matches(pid) behavior for
Some(pid).
In `@src/nfs.rs`:
- Around line 1247-1283: Add Linux-only unit tests that exercise the
/proc/mounts parsing and normalization code paths: create tests that call
unescape_proc_mounts and normalize_mount_path (and a small integration-style
test hitting is_mounted by feeding a fake /proc/mounts string if your test
harness can inject it, otherwise test the string-only logic directly) to assert
that octal escapes "\\040", "\\011", "\\012", "\\134" map to " ", tab, newline,
and "\" respectively and that trailing slashes are normalized (e.g., "/foo/" ->
"/foo", "/" stays "/"). Mark the tests with #[cfg(target_os = "linux")] and
include a table-driven set of cases for escapes and trailing-slash behavior so
the regression is covered by CI.
---
Outside diff comments:
In `@src/bin/hf-mount-gui/app.rs`:
- Around line 916-921: The forced unmount call in on_exit() that directly calls
platform::unmount_path(mount_point) can block shutdown; change this to perform
the fallback unmount off the shutdown thread by submitting it to a detached
worker or a timeout-aware helper: either spawn a detached thread
(std::thread::spawn) or use tokio::spawn_blocking to call
platform::unmount_path(mount_point) and return immediately, or wrap the blocking
call in a helper like platform::unmount_path_with_timeout(mount_point, Duration)
that runs the blocking unmount in a separate thread and enforces a timeout;
update the code that checks handle.is_finished() / self.active_mount_point in
on_exit() to call this nonblocking helper instead of calling
platform::unmount_path directly so on_exit() cannot be blocked by a wedged NFS
unmount.
In `@src/bin/hf-mount-gui/platform.rs`:
- Around line 138-145: The current open_mount_point uses
open_command(&target).spawn() which returns success as soon as the launcher
process starts; instead run the command to completion and return its exit status
so real launcher failures propagate. Replace the spawn() call with calling
.output() (or .status()) on the Command returned by open_command(&target), map
any Io error into the same Err path, then check the returned ExitStatus: if
status.success() return Ok(()), otherwise return Err with a descriptive message
including the exit code and (if using .output()) stderr/stdout text; keep
references to open_mount_point, open_target and open_command so the change is
applied in that function.
In `@src/nfs.rs`:
- Around line 840-884: The code currently logs NfsMountEvent::Mounted and calls
daemon_guard.notify_ready() unconditionally even when skip_auto_mount is true
(the branch that skips invoking mount.exe), which reports a live mount that
doesn't exist; modify the flow so those two actions only happen when a real
mount was performed—e.g. introduce a boolean like mount_performed (set true
after successful mount in the else branch where mount_windows_nfs_with_retry
returns Some and output.status.success()), and wrap the calls to
on_event(NfsMountEvent::Mounted { mount_point: mount_point_str.to_string() })
and daemon_guard.notify_ready() in a conditional that checks mount_performed (or
alternatively return early from the skip_auto_mount branch instead of falling
through).
- Around line 757-766: Change the concurrent-selects so a concurrently-ready
mount command always wins and thus the later unmount/spawn_unmount path runs: in
src/nfs.rs at 757-766 (macOS) and 795-804 (Linux) add "biased;" to the
tokio::select! and ensure the command arm (status = command.status() / output =
command.output()) is the first arm so it will be picked when both are ready
(leaving the existing server_handle.abort(),
on_event(NfsMountEvent::ShuttingDown { .. }), and vfs_for_shutdown.shutdown()
logic in the shutdown arm unchanged); do the same change for the Windows retry
helper at src/nfs.rs 1354-1356 (add "biased;" and make the command.output() arm
the first arm) so a simultaneous successful mount.exe result isn’t treated as
cancelled and spawn_unmount(...) still runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63b75688-3ac3-40e0-ac10-c6ac41fd737d
📒 Files selected for processing (5)
src/bin/hf-mount-gui/app.rssrc/bin/hf-mount-gui/platform.rssrc/bin/hf-mount-gui/profile.rssrc/bin/hf-mount-gui/worker.rssrc/nfs.rs
A review round on the GUI/NFS rewrite surfaced several ways a wedged or recycled NFS mount could hang the UI, orphan a mount, or misreport state. GUI (hf-mount-gui): - on_exit now runs the forced fallback unmount on a detached thread with a bounded wait, so window close stays bounded even if umount wedges. - Startup reconcile is non-blocking (heartbeat-only). The full liveness check stats the mount and must never run before the first frame; the poller re-confirms on its own thread within one interval. - worker_status_is_live treats terminal states as dead and no longer trusts the Windows metadata-only mount probe as authoritative (a leftover directory from a crashed worker could pass it); it falls through to the PID check there instead. - Post-stop cleanup only unmounts a path confirmed to be our loopback NFS export, so the race cleanup can't detach an unrelated pre-existing mount. NFS backend (src/nfs.rs): - biased mount-command selects so a mount that completed in the same poll as a stop wins; the wait loop then unmounts it instead of the server tearing down with the client mount left behind. - HF_MOUNT_SKIP_AUTO_MOUNT no longer emits Mounted or signals readiness. - Add is_loopback_nfs_mount plus Linux /proc/mounts parsing regression tests (octal-escape decode, trailing-slash normalization). Sidecar (hf-mount-fuse-sidecar): convert a last-resort panic in the mount thread into an error marker, so a panic before the ready/error file is written can't hang pod readiness (the readiness wait loop has no timeout). https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
2 participants
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.
Summary
The GUI binary did not compile at HEAD — an E0382 borrow-after-move in
start_mount(mount_pointmoved intoself.active_mount_point, then borrowed on the next line) shipped unnoticed becauseci.ymllints every feature combination exceptgui. This PR fixes the build, rewrites the GUI for a better UX and stability, hardens the library's mount-setup path, and closes the CI gap.GUI rewrite (
src/bin/hf-mount-gui/, replaces the 2,637-line single file)Layout / UX
Stability
tasklist.exeand stat-ed a possibly-wedged NFS drive on the UI thread — up to dozens of subprocess launches per second during interaction, and multi-second freezes on a hung mount.MountShutdownhandle: Stop now works while the mount command is still retrying, not only after a successful mount. Window close uses the same path with a bounded grace period plus a forced unmount fallback.New features
GetLogicalDrivessyscall — no per-letter probing that could hang).Library hardening
setup::build/build_with_runtimenow returnResultinstead of panicking on every config/auth error (newError::Setupvariant). The FUSE sidecar drops itscatch_unwind-and-parse-the-panic workaround; CLI binaries print a clean error and exit.nfs::mount_nfs_with_callbacktakes anNfsMountParamsstruct with an optionalMountShutdown; Linux/macOS mount commands now usetokio::process(no blocking the runtime); the mount-disappeared probe runs viaspawn_blocking; the Windows Error-53 retry loop no longer ends inunreachable!()and is cancellation-aware.hf_mount::windowsmodule deduplicates the drive-letter/System32 helpers previously copy-pasted innfs.rs,setup.rs, and the GUI; the pure parsing logic is now unit-tested on Linux CI.StagingDir::newreturnsResultinstead of panicking.CI
ci.ymlnow runscargo clippy --no-default-features --features nfs,gui --bins --tests -- -D warningsand the GUI unit tests on the Linux runner — this exact class of breakage is now caught on every PR without waiting for the Windows/macOS builders.Verification
cargo clippyclean with-D warningsacross all five feature combos (none, nfs, fuse, fuse+nfs, nfs+gui).cargo test --lib --features fuse,nfs: 356 passed.--features nfs(Windows CI config): 356 passed. GUI bin tests: 8 passed.cargo +nightly-2026-04-22 fmt --checkclean (also fixes pre-existing drift indaemon.rs).--version,--help,--check-setupexit codes) verified on Linux.windows-build.yml/macos-gui-build.ymlon this PR.https://claude.ai/code/session_01D3AdfdaNzP6eBm3seWGuJP
Generated by Claude Code