Fixed symlink support for config.toml #9445
Conversation
We already support reading from `config.toml` through a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles. This resolves #6646. Notes: * Symlink cycles or resolution failures replace the top-level symlink with a real file. * Shared config write path now handles symlinks consistently across edits, defaults, and empty-user-layer creation.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| pub write_path: PathBuf, | ||
| } | ||
|
|
||
| pub fn resolve_symlink_write_paths(path: &Path) -> io::Result<SymlinkWritePaths> { |
There was a problem hiding this comment.
Can you please add a docstring for this? It looks like it guards against cycles but has no max number of symlink resolutions?
Also, can you please add a unit test?
| } | ||
|
|
||
| async fn write_empty_user_config(write_path: PathBuf) -> Result<(), ConfigServiceError> { | ||
| task::spawn_blocking(move || write_atomically(&write_path, "")) |
There was a problem hiding this comment.
Should we make an async version of write_atomically() instead?
| std::fs::create_dir_all(parent)?; | ||
| let tmp = NamedTempFile::new_in(parent)?; | ||
| std::fs::write(tmp.path(), contents)?; | ||
| tmp.persist(write_path)?; |
There was a problem hiding this comment.
I guess this is the real reason why this needs to be sync? It doesn't appear as though there is an async version readily available...
Incidentally, here is how the tempfile crate creates a file on UNIX:
| Ok(normalize_for_wsl(canonical)) | ||
| } | ||
|
|
||
| pub struct SymlinkWritePaths { |
There was a problem hiding this comment.
This type feels a little weird to me: symlinks are relatively rare, but you get this type back even when symlinks are not involved.
Should this be an enum like:
pub enum WritePath {
RegularFile {
write_path: PathBuf,
}
Symlink {
read_path: PathBuf,
write_path: PathBuf,
}
}
We already support reading from
config.tomlthrough a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles.This resolves #6646.
Notes:
This PR was inspired by #9437, which was contributed by @ryoppippi