diff --git a/internal/shim/install.go b/internal/shim/install.go index e8460e0..d64b818 100644 --- a/internal/shim/install.go +++ b/internal/shim/install.go @@ -67,21 +67,29 @@ func Install(shims []string) (InstallResult, error) { } target := shimTargetPath(dir, name) - // Idempotency: if target already points at us, nothing to do. - if cur, err := os.Stat(target); err == nil { - if self, sErr := os.Stat(selfExe); sErr == nil && os.SameFile(cur, self) { - res.Skipped = append(res.Skipped, name+": already installed") - // Still try the legacy cleanup so a re-run after upgrade tidies stragglers. - if cleanupLegacyBareName(dir, name, selfExe) { - res.LegacyRemoved = append(res.LegacyRemoved, name) + // Detect anything already at target with Lstat, which (unlike os.Stat) + // does NOT follow the link — so a dangling/broken shim is seen too. + // os.Stat errors on a broken shim, which used to skip cleanup and make + // createShim fail with "file exists" on the next install. + if lfi, lerr := os.Lstat(target); lerr == nil { + // A symlink that still resolves to us? Idempotent — leave it. + if lfi.Mode()&os.ModeSymlink != 0 { + if cur, sErr := os.Stat(target); sErr == nil { + if self, e := os.Stat(selfExe); e == nil && os.SameFile(cur, self) { + res.Skipped = append(res.Skipped, name+": already installed") + // Still try the legacy cleanup so a re-run after upgrade tidies stragglers. + if cleanupLegacyBareName(dir, name, selfExe) { + res.LegacyRemoved = append(res.LegacyRemoved, name) + } + continue + } } - continue } - // Foreign file at our spot — remove it before relink. - _ = os.Remove(target) + // Foreign file, wrong target, or dangling shim — clear it before relinking. + if rmErr := os.Remove(target); rmErr != nil { + return res, fmt.Errorf("remove stale shim %s: %w", target, rmErr) + } } - // Stale entry that isn't a file but isn't a known link either (e.g., broken symlink). - // os.Stat above fails for broken symlinks; nothing to do then. if cleanupLegacyBareName(dir, name, selfExe) { res.LegacyRemoved = append(res.LegacyRemoved, name) @@ -123,7 +131,10 @@ func Uninstall() (InstallResult, error) { } for name := range KnownManagers { target := shimTargetPath(dir, name) - info, err := os.Stat(target) + + // Lstat so a dangling shim is detected too — os.Stat reports ENOENT for a + // broken link, which used to make us treat an existing shim as already gone. + lfi, err := os.Lstat(target) if errors.Is(err, os.ErrNotExist) { // Even if the canonical target is gone, a legacy bare-name shim may linger. if cleanupLegacyBareName(dir, name, selfExe) { @@ -135,7 +146,20 @@ func Uninstall() (InstallResult, error) { res.Skipped = append(res.Skipped, name+": "+err.Error()) continue } - if !os.SameFile(info, selfInfo) { + // Ours if it resolves to us, or if it's a symlink (valid OR dangling) + // pointing at the refuse binary in our own bin dir. Leave foreign files alone. + ours := false + if cur, sErr := os.Stat(target); sErr == nil { + ours = os.SameFile(cur, selfInfo) + } else if lfi.Mode()&os.ModeSymlink != 0 { + if dest, rlErr := os.Readlink(target); rlErr == nil { + if !filepath.IsAbs(dest) { + dest = filepath.Join(dir, dest) + } + ours = filepath.Dir(filepath.Clean(dest)) == dir + } + } + if !ours { res.Skipped = append(res.Skipped, name+": not our shim") continue } diff --git a/internal/shim/install_test.go b/internal/shim/install_test.go new file mode 100644 index 0000000..39e7ce3 --- /dev/null +++ b/internal/shim/install_test.go @@ -0,0 +1,75 @@ +package shim + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// A dangling shim (a symlink whose target no longer exists — e.g. left over +// after the refuse binary moved) must NOT block a re-install, and must be +// cleaned up by uninstall. Regression for: +// +// Error: create shim .../bin/npm: symlink ...: file exists +// +// which happened because the old code probed the shim with os.Stat (which +// follows the link and errors on a broken one), so the stale link was never +// cleared before os.Symlink. +func TestInstallReplacesDanglingShim(t *testing.T) { + // Shims are symlinks only on unix; on Windows createShim uses a + // hardlink/copy (and an extension), so the dangling-symlink scenario and + // these EvalSymlinks assertions don't apply. + if runtime.GOOS == "windows" { + t.Skip("shim relink test is unix-specific (windows shims are not symlinks)") + } + + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("REFUSE_HOME", "") // force the HOME-derived ~/.refuse path + + binPath := filepath.Join(home, ".refuse", "bin") + if err := os.MkdirAll(binPath, 0o755); err != nil { + t.Fatal(err) + } + + self, err := os.Executable() + if err != nil { + t.Fatal(err) + } + if r, e := filepath.EvalSymlinks(self); e == nil { + self = r + } + + npm := filepath.Join(binPath, "npm") + // Pre-seed a DANGLING npm shim pointing at a refuse that no longer exists. + if err := os.Symlink(filepath.Join(binPath, "refuse-gone"), npm); err != nil { + t.Fatal(err) + } + + if _, err := Install([]string{"npm"}); err != nil { + t.Fatalf("Install over a dangling shim failed: %v", err) + } + + resolved, err := filepath.EvalSymlinks(npm) + if err != nil { + t.Fatalf("npm shim still dangling after install: %v", err) + } + if resolved != self { + t.Errorf("npm shim resolves to %s, want %s", resolved, self) + } + + // Uninstall must remove a dangling shim too (one that points into our bin dir). + if err := os.Remove(npm); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(binPath, "refuse"), npm); err != nil { // dangling, in our dir + t.Fatal(err) + } + if _, err := Uninstall(); err != nil { + t.Fatalf("Uninstall failed: %v", err) + } + if _, err := os.Lstat(npm); !os.IsNotExist(err) { + t.Errorf("uninstall left the dangling shim behind (lstat err = %v)", err) + } +}