fix: clean up empty parent install dir on failed install#8302
Conversation
When both config.toml and config.local.toml exist, `mise use` was incorrectly writing to config.local.toml. Added is_conf_d_file() helper and updated first_config_file() to skip conf.d files, then updated global_config_path() to use first_config_file() instead of .last() so it selects config.toml over config.local.toml. Fixes: #8236 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When an install fails (e.g. due to a GitHub rate limit or a 404 for a non-existent version), cleanup_install_dirs_on_error only removed the version-specific subdirectory (e.g. installs/tilt/latest) but left the parent directory (installs/tilt/) as an empty directory. Fix by also removing the parent installs_path when it is empty after cleanup, and clean up the incomplete marker from cache. Adds e2e regression test using a non-existent tilt version to trigger the same 404 failure path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello @jdx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request addresses an issue where failed installations would leave empty parent directories and incomplete markers. It introduces logic to clean up these artifacts and includes new e2e tests to verify the fix. Additionally, it corrects the behavior of mise use and mise use -g to write to config.toml instead of config.local.toml when appropriate, and updates the first_config_file logic to prioritize non-conf.d files. The changes are well-tested with new e2e tests and seem to resolve the reported issues effectively. Feedback includes suggestions for safer error handling in filesystem operations and improved code organization for helper functions.
src/backend/mod.rs
Outdated
| if let Ok(entries) = file::dir_subdirs(installs_path) { | ||
| if entries.is_empty() { | ||
| let _ = remove_all_with_warning(installs_path); | ||
| } | ||
| } |
There was a problem hiding this comment.
The file::dir_subdirs function returns a Result<BTreeSet<String>>. It's safer to handle the Err case explicitly rather than unwrapping, especially when dealing with filesystem operations that can fail due to permissions or other issues. If dir_subdirs returns an error, entries would be an empty set, which would lead to remove_all_with_warning being called on installs_path even if there are other files/directories that just couldn't be read.
if let Ok(entries) = file::dir_subdirs(installs_path) {
if entries.is_empty() {
let _ = remove_all_with_warning(installs_path);
}
} else {
warn!("Failed to read subdirectories of {}: {:#}", installs_path.display(), "error");
}There was a problem hiding this comment.
Pull request overview
Fixes install cleanup behavior so failed installs don’t leave behind empty parent install directories (and related cache markers), and adjusts global config selection so mise use -g prefers writing to config.toml over config.local.toml.
Changes:
- Remove incomplete cache marker and delete the tool’s parent installs directory when it becomes empty after a failed install.
- Change global config path selection to prefer the first non-local TOML (skipping
.tool-versionsandconf.dentries). - Add/extend e2e coverage for failed install cleanup and
mise useglobal/local config write targets.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/config/mod.rs | Updates global config selection logic and adds conf.d filtering for config file picking. |
| src/backend/mod.rs | Enhances failed-install cleanup to remove incomplete markers and empty parent installs dirs. |
| e2e/cli/test_use | Adds e2e assertions that mise use writes to config.toml (not config.local.toml). |
| e2e/backend/test_aqua_failed_install_cleanup | Adds regression test ensuring empty parent install dirs are removed after failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn first_config_file(files: &IndexSet<PathBuf>) -> Option<&PathBuf> { | ||
| files | ||
| .iter() | ||
| .find(|p| !is_tool_versions_file(p)) | ||
| .find(|p| !is_tool_versions_file(p) && !is_conf_d_file(p)) | ||
| .or_else(|| files.first()) | ||
| } |
There was a problem hiding this comment.
first_config_file() depends on the iteration order of IndexSet<PathBuf> built from directory listing(s). If config_files_from_dir() ultimately relies on read_dir iteration order, the chosen file can be nondeterministic (and the e2e assertions about preferring config.toml over config.local.toml can become flaky). Consider making the selection deterministic by explicitly preferring config.toml over config.local.toml (and/or sorting candidates by a stable key) instead of taking the first match.
src/backend/mod.rs
Outdated
| && let Ok(entries) = file::dir_subdirs(installs_path) | ||
| && entries.is_empty() { | ||
| let _ = remove_all_with_warning(installs_path); | ||
| } |
There was a problem hiding this comment.
The emptiness check uses dir_subdirs(installs_path) and removes installs_path when there are no subdirectories, but the directory may still contain non-directory entries (e.g., marker files, metadata, platform-specific files). In that case this would delete a non-empty directory. Prefer checking that the directory has no entries at all (e.g., via a generic read_dir-based emptiness check) before deleting it.
| && let Ok(entries) = file::dir_subdirs(installs_path) | |
| && entries.is_empty() { | |
| let _ = remove_all_with_warning(installs_path); | |
| } | |
| && let Ok(mut entries) = std::fs::read_dir(installs_path) | |
| && entries.next().is_none() | |
| { | |
| let _ = remove_all_with_warning(installs_path); | |
| } |
| # Trigger a failed install using a non-existent version number. | ||
| # This causes the GitHub API to return 404 after create_install_dirs() has | ||
| # already created installs/tilt/9999.9999.9999. | ||
| assert_fail "mise x tilt@9999.9999.9999 -- tilt version" |
There was a problem hiding this comment.
This regression test can become a false-positive if the command fails before create_install_dirs() runs (e.g., due to an unrelated network/auth issue), because $MISE_DATA_DIR/installs/tilt may never be created and the assertion would still pass. To make the test robust, assert that the failure is specifically the expected 404/non-existent version error (e.g., by matching stderr/stdout for the expected message/status) before checking directory cleanup.
| assert_fail "mise x tilt@9999.9999.9999 -- tilt version" | |
| output="$(mise x tilt@9999.9999.9999 -- tilt version 2>&1)" | |
| status=$? | |
| if [ "$status" -eq 0 ]; then | |
| echo "Expected 'mise x tilt@9999.9999.9999 -- tilt version' to fail, but it succeeded." >&2 | |
| exit 1 | |
| fi | |
| if ! grep -qiE '404|not found' <<<"$output"; then | |
| echo "Expected a 404/non-existent version error, but got:" >&2 | |
| echo "$output" >&2 | |
| exit 1 | |
| fi |
|
bugbot run |
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.18 x -- echo |
23.6 ± 0.4 | 22.7 | 25.7 | 1.00 |
mise x -- echo |
24.0 ± 0.5 | 23.0 | 26.5 | 1.02 ± 0.03 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.18 env |
22.3 ± 0.6 | 21.6 | 28.3 | 1.00 |
mise env |
22.7 ± 0.5 | 21.7 | 24.6 | 1.02 ± 0.04 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.18 hook-env |
23.0 ± 0.5 | 22.1 | 28.1 | 1.00 |
mise hook-env |
23.2 ± 0.4 | 22.4 | 25.8 | 1.01 ± 0.03 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.18 ls |
20.6 ± 0.5 | 19.7 | 23.5 | 1.00 |
mise ls |
20.9 ± 0.6 | 19.9 | 23.1 | 1.01 ± 0.04 |
xtasks/test/perf
| Command | mise-2026.2.18 | mise | Variance |
|---|---|---|---|
| install (cached) | 125ms | 124ms | +0% |
| ls (cached) | 77ms | 77ms | +0% |
| bin-paths (cached) | 82ms | 80ms | +2% |
| task-ls (cached) | 811ms | 808ms | +0% |
### 🐛 Bug Fixes - **(docs)** correct ripgrep command by @nguyenvulong in [#8299](#8299) - **(task)** skip setpgid for TTY stdin to fix interactive tasks by @jdx in [#8301](#8301) - clean up empty parent install dir on failed install by @jdx in [#8302](#8302) ### Chore - **(release)** run communique via mise x for PATH resolution by @jdx in [#8294](#8294) ## 📦 Aqua Registry Updates #### New Packages (2) - [`kubie-org/kubie`](https://github.com/kubie-org/kubie) - [`steipete/gogcli`](https://github.com/steipete/gogcli)
## Summary - When an install fails (e.g. GitHub rate limit 403, or a non-existent version 404), `cleanup_install_dirs_on_error` only removed the version-specific subdirectory (e.g. `installs/tilt/latest`) but left the parent directory (`installs/tilt/`) as an empty directory - Fix by also removing the parent `installs_path` when it is empty after cleanup, guarded so it won't remove it if other versions exist - Also cleans up the incomplete marker from cache (was also left behind on error) - Adds e2e regression test: installs a non-existent tilt version to trigger a 404 failure, asserts the parent dir is gone, then verifies a real install still works ## Test plan - [x] `mise run test:e2e test_aqua_failed_install_cleanup` passes - [x] Test fails without the patch (confirmed via `git stash`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches install cleanup and config file selection logic; mistakes could delete unintended directories or write tool versions to the wrong config file, though changes are guarded and covered by new e2e tests. > > **Overview** > Fixes failed-install cleanup so `cleanup_install_dirs_on_error` also removes the cache “incomplete” marker and deletes the tool’s parent installs directory when it becomes empty, preventing leftover empty `installs/<tool>/` folders. > > Refines config write targeting by updating `first_config_file`/`global_config_path` to skip `conf.d` entries (and `.tool-versions`) so `mise use` and `mise use -g` prefer writing to `config.toml` over `config.local.toml`. Adds e2e coverage for both the failed-install directory cleanup regression and the global/local config write preference. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 901bf70. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 🐛 Bug Fixes - **(docs)** correct ripgrep command by @nguyenvulong in [jdx#8299](jdx#8299) - **(task)** skip setpgid for TTY stdin to fix interactive tasks by @jdx in [jdx#8301](jdx#8301) - clean up empty parent install dir on failed install by @jdx in [jdx#8302](jdx#8302) ### Chore - **(release)** run communique via mise x for PATH resolution by @jdx in [jdx#8294](jdx#8294) ## 📦 Aqua Registry Updates #### New Packages (2) - [`kubie-org/kubie`](https://github.com/kubie-org/kubie) - [`steipete/gogcli`](https://github.com/steipete/gogcli)
Summary
cleanup_install_dirs_on_erroronly removed the version-specific subdirectory (e.g.installs/tilt/latest) but left the parent directory (installs/tilt/) as an empty directoryinstalls_pathwhen it is empty after cleanup, guarded so it won't remove it if other versions existTest plan
mise run test:e2e test_aqua_failed_install_cleanuppassesgit stash)🤖 Generated with Claude Code
Note
Medium Risk
Touches install cleanup and config file selection logic; mistakes could delete unintended directories or write tool versions to the wrong config file, though changes are guarded and covered by new e2e tests.
Overview
Fixes failed-install cleanup so
cleanup_install_dirs_on_erroralso removes the cache “incomplete” marker and deletes the tool’s parent installs directory when it becomes empty, preventing leftover emptyinstalls/<tool>/folders.Refines config write targeting by updating
first_config_file/global_config_pathto skipconf.dentries (and.tool-versions) somise useandmise use -gprefer writing toconfig.tomloverconfig.local.toml. Adds e2e coverage for both the failed-install directory cleanup regression and the global/local config write preference.Written by Cursor Bugbot for commit 901bf70. This will update automatically on new commits. Configure here.