fix(mounts): resolve rclone remote names consistently#3624
Conversation
- Use `RcloneMountPattern.resolve_remote_name()` when building the per-session config path and when launching/unmounting rclone processes. - This keeps `apply()` and `unapply()` aligned on the same remote name, preserves deterministic naming when no explicit remote_name is provided, and removes the current dead wiring around pattern-level name resolution. - The change is intentionally limited to name resolution and does not alter mount semantics.
![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: e210af4418
ℹ️ 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".
| "rclone", | ||
| "mount", | ||
| f"{config.remote_name}:{config.remote_path}", | ||
| f"{remote_name}:{config.remote_path}", |
There was a problem hiding this comment.
When any rclone mount is applied, the default fuse path calls _start_rclone_client(), but this helper has no remote_name parameter or local binding, so evaluating this f-string raises NameError before rclone is invoked. The NFS server helper has the same issue, so this change currently breaks rclone mounts instead of just changing name resolution.
Useful? React with 👍 / 👎.
| remote_name = self.resolve_remote_name( | ||
| session_id=session_id.hex, | ||
| remote_kind=rclone_config.remote_kind, | ||
| mount_type=rclone_config.mount_type, | ||
| ) |
There was a problem hiding this comment.
For mounts where the pattern-level or derived name differs from rclone_config.remote_name, teardown computes the resolved remote_name here but the pkill pattern below still matches rclone_config.remote_name. After apply launches rclone under the resolved name, unapply will miss that process and leave the rclone mount/server running.
Useful? React with 👍 / 👎.
- Thread the resolved rclone remote name through `_start_rclone_server()`, `_start_rclone_client()`, and `unapply()` so mount creation and cleanup use the same identifier. - This fixes the regression introduced by the name-resolution change, prevents the helper NameError, and keeps teardown aligned with the process started during `apply()`. - No public API changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9646e6dba4
ℹ️ 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".
| remote_name = self.resolve_remote_name( | ||
| session_id=session_id_str, | ||
| remote_kind=rclone_config.remote_kind, | ||
| mount_type=rclone_config.mount_type, | ||
| ) |
There was a problem hiding this comment.
When RcloneMountPattern.apply() is called with a caller-supplied RcloneMountConfig whose config_text defines the section named by rclone_config.remote_name (the existing direct/custom adapter path), this now ignores that name and launches rclone with a generated sandbox_<kind>_<session> remote instead. The isolated config file is still written from the original config_text, so rclone looks for a section that is not present and the mount fails (for example [remote] is written but sandbox_s3_...:bucket is mounted).
Useful? React with 👍 / 👎.
- Pass the configured rclone remote name through the helper launch path and reuse it in `unapply()` so mount setup and cleanup target the same remote. - This keeps the current config-file-based adapter path working without changing user-facing behavior.
2 participants
Use
RcloneMountPattern.resolve_remote_name()when building the per-session config path and when launching/unmounting rclone processes.This keeps
apply()andunapply()aligned on the same remote name, preserves deterministic naming when no explicit remote_name is provided, and removes the current dead wiring around pattern-level name resolution.The change is intentionally limited to name resolution and does not alter mount semantics.