Skip to content

fix(install): stop rewriting healthy runtime symlinks#9410

Merged
jdx merged 2 commits intomainfrom
claude/wizardly-goldstine-2946f4
Apr 26, 2026
Merged

fix(install): stop rewriting healthy runtime symlinks#9410
jdx merged 2 commits intomainfrom
claude/wizardly-goldstine-2946f4

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Apr 26, 2026

Summary

Fixes the mise install regression where a non-root user gets Permission denied while 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 runs mise install at runtime). Reported in #8596 and addressed by #9408 — this PR is an alternative.

Root cause

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:

if from.exists() {
    if is_runtime_symlink(&from) && file::resolve_symlink(&from)?.unwrap_or_default() != to {
        // Branch 1 — only fires when the symlink target is wrong
        ...
    } else if from.file_name() != to.file_name() && !concrete_installs.contains(&from_name) {
        // Branch 2 — fires for ANY symlink because file_name("latest") != file_name("22.5.0")
        file::remove_all(&from)?;
    }
    ...
}

For a perfectly healthy latest -> ./22.5.0:

  • Branch 1 fails (target == to).
  • Branch 2 enters because from.file_name() ("latest") differs from to.file_name() ("22.5.0") and concrete_installs doesn't include "latest".
  • remove_all then runs on a healthy symlink — wasted I/O on every install in the common case, hard failure when the dir is read-only.

Fix

  • Branch 1 now handles all runtime-symlink cases: rewrite if target differs, otherwise continue.
  • Branch 2 only fires for non-symlink stale dirs (its actual purpose — cleanup for the 2026.4 regression that created real latest/ directories).
  • Drop the position-based permission tolerance added in fix(install): create runtime symlinks in system/shared install directories #8722. With healthy symlinks taking the no-op path, read-only system dirs aren't touched at all. If a write IS genuinely required and can't happen, fail loudly so the user knows to fix permissions rather than silently ending up with a stale latest after mise install --system.

Why not #9408

That PR replaces position-based tolerance with category-based tolerance (any non-Local dir gets permission errors swallowed) and silently skips system/shared installs from user shims. Both:

  • Mask the real bug (the false-positive remove_all above) instead of fixing it. Even with category-based tolerance the install does I/O it shouldn't, just hidden under a debug!.
  • Trade one form of silent behavior for another. Reviewers (greptile, gemini) flagged the shims change as a regression for the "root installs once, all users get shims" pattern.
  • Don't match the maintainer's stated policy: if mise install doesn'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 up tiny@1.0.0 in user dir, tiny@2.1.0 in a fake system dir, chmod -R a-w the system dir, then runs mise install --force tiny@1.0.0 and mise install dummy@1.0.0. Asserts both succeed AND that the skipping symlink update / Permission denied / failed rm warnings do NOT appear in output. Verified to fail before this fix (with WARN skipping symlink update for ...: failed rm -rf ... Permission denied) and pass after.

Test plan

  • cargo test --bin mise — 774 pass
  • mise run lint-fix — clean
  • mise run test:e2e test_install_system_readonly — pass
  • mise 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 install from rewriting existing runtime symlinks when they already point at the desired target, avoiding unnecessary deletes/recreates that previously caused Permission denied failures on read-only system install directories.

Refactors shared/system install-dir enumeration into install_dirs_for and 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.

`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>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/runtime_symlinks.rs Outdated
@@ -103,6 +75,8 @@ fn rebuild_symlinks_in_dir(
.is_some_and(|(from_name, to_name)| from_name != to_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
.is_some_and(|(from_name, to_name)| from_name != to_name)
.is_some_and(|(f, t)| f != t)

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 26, 2026

Greptile Summary

Fixes the false-positive remove_all on healthy runtime symlinks in rebuild_symlinks_in_dir — the old code fell through to branch 2 for any correctly-pointing latest → ./X.Y.Z symlink because "latest" != "X.Y.Z" always, causing unnecessary writes that failed against read-only system dirs. The fix restructures the branch so existing runtime symlinks with a matching target immediately continue, and removes the now-unnecessary category-based permission-error swallowing in rebuild/migrate_real_dirs. An e2e test (test_install_system_readonly) is added to guard the regression.

Confidence Score: 5/5

Safe 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 else if branch (branch 2) fired on every healthy latest symlink because "latest" != "X.Y.Z" is always true, causing remove_all on a non-stale path. The new is_runtime_symlink guard with early continue closes this gap precisely. Removal of is_permission_error is clean since the no-op path means read-only dirs are never touched unless a genuine write is needed. The e2e test properly captures exit codes via if ! out="$(...)", addressing prior review feedback.

No files require special attention.

Important Files Changed

Filename Overview
src/runtime_symlinks.rs Core fix: rebuild_symlinks_in_dir now short-circuits with continue when a runtime symlink already points to the correct target, preventing spurious remove_all on healthy symlinks; is_permission_error removed; permission-error tolerance replaced by delta-based no-op semantics.
e2e/cli/test_install_system_readonly New regression test: sets up user + read-only system installs, verifies mise install completes without warnings/permission errors; assert_silent_install helper correctly captures both stdout/stderr and exit code.

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]
Loading

Reviews (2): Last reviewed commit: "review: rename shadowed closure params a..." | Re-trigger Greptile

Comment thread e2e/cli/test_install_system_readonly Outdated
Comment thread e2e/cli/test_install_system_readonly Outdated
- 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>
@jdx
Copy link
Copy Markdown
Owner Author

jdx commented Apr 26, 2026

Addressed review feedback in acf5798:

  • gemini's note on shadowing in rebuild_symlinks_in_dir: not actually a compile error — the concrete_installs.contains(&from_name) call sits outside the closure and binds to the outer from_name: String (which is why cargo check and the test suite both pass on the original commit). But the shadowing IS confusing to read, so renamed the closure params to f/t.
  • greptile's exit-code check on test_install_system_readonly: valid — out=\"\$(mise install ...)\" discards the exit code, so a mise crash with no matching warning string would slip through (the symlink asserts at the end would catch some final-state regressions, but not all). Pulled both checks into an assert_silent_install helper that fails on a non-zero exit.

This comment was generated by an AI coding assistant.

@github-actions
Copy link
Copy Markdown

Hyperfine Performance

mise x -- echo

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%

@jdx jdx merged commit a0eff6d into main Apr 26, 2026
37 checks passed
@jdx jdx deleted the claude/wizardly-goldstine-2946f4 branch April 26, 2026 19:29
jdx pushed a commit that referenced this pull request Apr 26, 2026
### 🚀 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant