fix(install): stop rewriting healthy runtime symlinks#9410
Conversation
`rebuild_symlinks_in_dir` always called `remove_all` + `make_symlink_or_file`
on every existing runtime symlink, because the "stale runtime dir" branch
fired for symlinks too. The conditions only excluded mismatched targets
(branch 1) and concrete-install names (branch 2's allowlist), so a healthy
`latest -> ./22.5.0` symlink fell through to branch 2: `from.file_name()`
("latest") differed from `to.file_name()` ("22.5.0"), `concrete_installs`
didn't include "latest", and `remove_all` ran on a perfectly fine symlink.
This was wasted I/O in the common case and a hard failure when the install
dir was read-only — the scenario from #8596: a non-root user runs
`mise install <project tool>` against a system installs dir that root
populated at Docker build time.
Branch 1 now handles all runtime-symlink cases (rewrite if target differs,
otherwise `continue`); branch 2 only fires for non-symlink stale dirs (the
2026.4 regression cleanup it was actually written for).
The position-based permission tolerance added in #8722 is no longer needed:
with healthy symlinks taking the no-op path, read-only system dirs don't
get touched at all. Drop the silent skip — if a write is genuinely
required and we can't make it, fail loudly so the user knows to fix
permissions rather than discovering later that their `--system` install
didn't update `latest`.
Closes #9408
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the runtime symlink management to avoid unnecessary write operations, specifically addressing issues in read-only system installation environments. It introduces a helper function to consolidate installation directory logic and adds a regression test. A critical issue was identified in src/runtime_symlinks.rs where variable shadowing in a closure causes a compilation error during a HashSet lookup.
| @@ -103,6 +75,8 @@ fn rebuild_symlinks_in_dir( | |||
| .is_some_and(|(from_name, to_name)| from_name != to_name) | |||
There was a problem hiding this comment.
The closure parameters from_name and to_name shadow the outer from_name variable (defined at line 62). This causes a compilation error at line 76 because concrete_installs is a HashSet<String>, and HashSet<String>::contains cannot be called with an &OsStr (the type of the shadowed from_name). Renaming these closure parameters will resolve the conflict and allow the check against concrete_installs to use the correct String variable.
| .is_some_and(|(from_name, to_name)| from_name != to_name) | |
| .is_some_and(|(f, t)| f != t) |
Greptile SummaryFixes the false-positive Confidence Score: 5/5Safe to merge — logic change is minimal and precisely targets the false-positive branch; new e2e test covers the regression scenario. No P0 or P1 findings. The Rust fix is logically correct: the old No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rebuild_symlinks_in_dir] --> B{from.exists?}
B -- No --> G[make_symlink_or_file]
B -- Yes --> C{is_runtime_symlink?}
C -- Yes --> D{resolve_symlink == to?}
D -- Yes --> E[continue / no-op ✅]
D -- No --> F[remove_file]
F --> G
C -- No --> H{file_name != to_name AND not concrete_install?}
H -- Yes --> I[remove_all stale dir]
I --> G
H -- No --> J[continue / no-op]
Reviews (2): Last reviewed commit: "review: rename shadowed closure params a..." | Re-trigger Greptile |
- runtime_symlinks: rename closure params `from_name`/`to_name` to `f`/`t` so the outer `from_name: String` used by `concrete_installs.contains` is obviously distinct from the closure's `OsStr` args. - test_install_system_readonly: extract `assert_silent_install` helper that fails on a non-zero exit code instead of just inspecting stdout. Catches the case where mise crashes with an unrelated error message. Review feedback from gemini-code-assist and greptile-apps on #9410. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed review feedback in acf5798:
This comment was generated by an AI coding assistant. |
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.22 x -- echo |
18.1 ± 0.4 | 17.0 | 20.2 | 1.00 |
mise x -- echo |
18.5 ± 0.4 | 17.4 | 19.8 | 1.02 ± 0.03 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.22 env |
17.7 ± 0.6 | 16.8 | 24.6 | 1.00 |
mise env |
18.0 ± 0.4 | 17.0 | 19.4 | 1.02 ± 0.04 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.22 hook-env |
18.1 ± 0.4 | 17.1 | 19.6 | 1.00 |
mise hook-env |
18.4 ± 0.4 | 17.5 | 19.6 | 1.02 ± 0.03 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.22 ls |
19.2 ± 0.5 | 18.1 | 21.6 | 1.00 |
mise ls |
19.6 ± 0.5 | 18.5 | 24.8 | 1.02 ± 0.04 |
xtasks/test/perf
| Command | mise-2026.4.22 | mise | Variance |
|---|---|---|---|
| install (cached) | 123ms | 122ms | +0% |
| ls (cached) | 68ms | 69ms | -1% |
| bin-paths (cached) | 68ms | 69ms | -1% |
| task-ls (cached) | 712ms | 705ms | +0% |
### 🚀 Features - **(backend)** add global libc preference by @jdx in [#9404](#9404) - opt-in to pre-release versions for github and aqua backends by @jakedgy in [#9329](#9329) ### 🐛 Bug Fixes - **(backend)** allow unresolved latest opt-in by @jdx in [#9401](#9401) - **(install)** stop rewriting healthy runtime symlinks by @jdx in [#9410](#9410) - **(node)** route musl tarball URLs to unofficial-builds by @jdx in [#9409](#9409) - **(prune)** skip remote version resolution for tracked configs by @jdx in [#9406](#9406) - **(schema)** allow array values in tool additionalProperties by @JP-Ellis in [#9400](#9400) ### 📦️ Dependency Updates - bump communique to 1.1.2 by @jdx in [#9402](#9402) ### 📦 Registry - use aqua for rumdl by @scop in [#9397](#9397) ### Chore - **(ci)** improve pr-closer workflow by @jdx in [#9403](#9403) - **(release)** stop appending sponsor blurb when communique succeeds by @jdx in [#9395](#9395) ### New Contributors - @JP-Ellis made their first contribution in [#9400](#9400)
Summary
Fixes the
mise installregression where a non-root user getsPermission deniedwhile rebuilding runtime symlinks against a read-only system installs dir (typical Docker pattern: root populates/usr/local/share/mise/installs/at build time, non-root user runsmise installat runtime). Reported in #8596 and addressed by #9408 — this PR is an alternative.Root cause
rebuild_symlinks_in_diralways calledremove_all+make_symlink_or_fileon every existing runtime symlink, because the "stale runtime dir" branch fired for symlinks too:For a perfectly healthy
latest -> ./22.5.0:target == to).from.file_name()("latest") differs fromto.file_name()("22.5.0") andconcrete_installsdoesn't include"latest".remove_allthen runs on a healthy symlink — wasted I/O on every install in the common case, hard failure when the dir is read-only.Fix
continue.latest/directories).latestaftermise install --system.Why not #9408
That PR replaces position-based tolerance with category-based tolerance (any non-
Localdir gets permission errors swallowed) and silently skips system/shared installs from user shims. Both:remove_allabove) instead of fixing it. Even with category-based tolerance the install does I/O it shouldn't, just hidden under adebug!.mise installdoesn't touch system tools, it should work fine; if it does and the dir isn't writable, it should fail loudly. This PR achieves both via a delta-based no-op rather than tolerance.Tests
e2e/cli/test_install_system_readonly— sets uptiny@1.0.0in user dir,tiny@2.1.0in a fake system dir,chmod -R a-wthe system dir, then runsmise install --force tiny@1.0.0andmise install dummy@1.0.0. Asserts both succeed AND that theskipping symlink update/Permission denied/failed rmwarnings do NOT appear in output. Verified to fail before this fix (withWARN skipping symlink update for ...: failed rm -rf ... Permission denied) and pass after.Test plan
cargo test --bin mise— 774 passmise run lint-fix— cleanmise run test:e2e test_install_system_readonly— passmise run test:e2e test_install_system test_shared_install_dirs test_install_before_ignores_stale_latest_dirs test_upgrade_latest_stale test_reshim_with_shims_on_path test_uninstall test_install_before test_install_short_ignores_full_backend_config test_install_dry_run test_install_concurrent_via_exec— all pass🤖 Generated with Claude Code
Note
Medium Risk
Changes runtime symlink rebuild behavior across both user and shared/system install dirs, which could affect installs/reshims if edge cases exist. It also removes permission-error swallowing for shared dirs, so genuine mismatches in read-only system installs will now fail loudly.
Overview
Prevents
mise installfrom rewriting existing runtime symlinks when they already point at the desired target, avoiding unnecessary deletes/recreates that previously causedPermission deniedfailures on read-only system install directories.Refactors shared/system install-dir enumeration into
install_dirs_forand removes the prior "skip on permission error" behavior; shared/system dirs are now treated like user dirs, but the rebuild path should be a no-op unless a change is actually needed. Adds an E2E regression test (test_install_system_readonly) that simulates a read-only system installs dir and asserts installs succeed without system-dir warnings.Reviewed by Cursor Bugbot for commit acf5798. Bugbot is set up for automated code reviews on this repo. Configure here.