From ee7e8dd952f8580578ab1d75cc784ef36abadf40 Mon Sep 17 00:00:00 2001 From: Florian Schaupp Date: Fri, 19 Jun 2026 22:21:52 +0200 Subject: [PATCH 1/2] Fix silent extraction failure masking failed install (#1357) nvm install could report "Installation complete" while leaving an empty or missing version directory, so nvm use failed with "Version not installed". The install path swallowed extraction failures: - GetNodeJS ignored fs.Move errors (only printed them) and still returned success, then RemoveAll deleted the un-moved files. Move errors are now propagated and node.exe is verified before reporting success. - install() reported the temp->root rename failure with the wrong (nil) error variable, so a failed final move was silently ignored. It now reports the real error and aborts. - The bundled-npm path fell through into the standalone-npm download path, operating on a temp directory that had just been moved away. It now returns once the bundled npm is in place. - rollback() only stat'd the version directory and never removed it; a failed or canceled install now actually cleans up the partial directory. This commonly triggers when the system drive is low on disk space during extraction. Co-Authored-By: Claude Opus 4.8 --- src/nvm.go | 19 ++++++++++++++++++- src/web/web.go | 22 ++++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/nvm.go b/src/nvm.go index 7a442222..107ac71d 100644 --- a/src/nvm.go +++ b/src/nvm.go @@ -412,6 +412,16 @@ func rollback(version string) error { writeToErrorLog(err) return fmt.Errorf("Error rolling back node v%s installation: %v.", version, err) } + + // Nothing to roll back - the version directory was never created. + return nil + } + + // Remove the partially-installed version directory so a failed or canceled + // install doesn't leave a broken version behind for `nvm use` to pick up. + if err := os.RemoveAll(p); err != nil { + writeToErrorLog(err) + return fmt.Errorf("Error rolling back node v%s installation: %v.", version, err) } return nil @@ -712,7 +722,8 @@ func install(version string, cpuarch string) { if file.Exists(filepath.Join(root, "v"+version, "node_modules", "npm")) { utility.DebugLogf("move %v to %v", filepath.Join(root, "v"+version), filepath.Join(env.root, "v"+version)) if rnerr := utility.Rename(filepath.Join(root, "v"+version), filepath.Join(env.root, "v"+version)); rnerr != nil { - status <- Status{Err: err} + status <- Status{Err: fmt.Errorf("failed to move node v%s into %s: %v", version, env.root, rnerr)} + return } utility.DebugFn(func() { utility.DebugLogf("env root: %v", env.root) @@ -733,6 +744,12 @@ func install(version string, cpuarch string) { npmv := getNpmVersion(version) status <- Status{Text: fmt.Sprintf("npm v%s installed successfully.\n\nIf you want to use this version, type\n\nnvm use %s", npmv, version), Done: true} } + + // npm was bundled with the Node.js archive and is already in place. + // Return here so we don't fall through into the standalone-npm + // download path below, which operates on the temp directory that + // was just moved out from under us. + return } // If successful, add npm diff --git a/src/web/web.go b/src/web/web.go index 7900e8c9..a2049305 100644 --- a/src/web/web.go +++ b/src/web/web.go @@ -285,9 +285,17 @@ func GetNodeJS(root string, v string, a string, append bool) bool { zip := root + "\\v" + v + "\\" + strings.Replace(filepath.Base(url), ".zip", "", 1) utility.DebugLogf("moving %v to %v", zip, root+"\\v"+v) - err = fs.Move(zip, root+"\\v"+v, true) + // Lift the extracted files out of the nested "node-vX-win-arch" folder + // into the version root. Do NOT ignore errors here: if a file fails to + // move (e.g. out of disk space, locked by antivirus, permission issue) + // we must surface it, otherwise the RemoveAll below would silently + // delete the un-moved files while the caller still reports a successful + // install (see issue #1357). + err = fs.Move(zip, root+"\\v"+v, false) if err != nil { - fmt.Println("ERROR moving file: " + err.Error()) + fmt.Println("Error extracting from Node archive (failed to move files into place): " + err.Error()) + os.RemoveAll(root + "\\v" + v) + return false } utility.DebugLog("move succeeded") @@ -306,6 +314,16 @@ func GetNodeJS(root string, v string, a string, append bool) bool { utility.DebugLog(string(out)) } }) + + // Verify the extraction actually produced a usable node binary before + // declaring success. Without this, a partial/blocked extraction is + // reported as "Complete" and the user ends up with an empty version + // directory (issue #1357). + if !file.Exists(root + "\\v" + v + "\\node.exe") { + fmt.Println("Error: extraction completed but node.exe is missing. The download may be corrupt or extraction was blocked.") + os.RemoveAll(root + "\\v" + v) + return false + } } fmt.Println("Complete") return true From 31c521158849c3ba5e1f772caaed386e124f6ca5 Mon Sep 17 00:00:00 2001 From: Florian Schaupp Date: Fri, 19 Jun 2026 23:07:11 +0200 Subject: [PATCH 2/2] Fix install leaving extracted files stranded in temp dir (#1357) The install flow extracts Node into a temp directory and then moves it into the nvm root with utility.Rename. Rename only fell back to a copy+delete when the source and destination were on different volumes; on the (common) same-volume case it did a bare os.Rename and returned any error verbatim. On Windows that rename frequently fails right after an unzip because a file in the tree is momentarily locked (e.g. antivirus scanning the freshly-written 100MB+ node.exe), or simply cannot be moved as a directory entry. The maintainers already worked around this for the standalone-npm move with an exponential backoff, but the temp->root moves had no such handling. The result: the version directory was never created, the fully-extracted files were left behind in %TEMP%\nvm-install-*, and nvm still reported a successful install. Make Rename robust for every caller: - retry the in-volume os.Rename with a short backoff to ride out the transient post-unzip lock window, then - fall back to a recursive copy + delete (the existing cross-volume path) when the rename still fails. This matches the manual workaround reported in the issue (copy the extracted folder into the nvm root) and complements the earlier change that stopped the failure from being silent. Co-Authored-By: Claude Opus 4.8 --- src/utility/rename.go | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/src/utility/rename.go b/src/utility/rename.go index ec5039a7..edfbded3 100644 --- a/src/utility/rename.go +++ b/src/utility/rename.go @@ -5,16 +5,48 @@ import ( "io" "os" "path/filepath" + "time" ) +// Rename moves old to new. +// +// It first attempts a cheap filesystem rename, which only works when the source +// and destination live on the same volume. That rename can still fail on +// Windows when a file in the source tree is briefly locked right after an unzip +// (e.g. antivirus scanning a freshly-extracted 100MB+ node.exe), so it is +// retried with a short backoff. If the rename ultimately fails — whether because +// the paths are on different volumes or because the directory entry simply +// cannot be moved — it falls back to a recursive copy + delete, which opens each +// file individually instead of relying on a single atomic directory rename. +// +// This makes the temp->install-root move reliable. Previously a same-volume +// os.Rename failure was returned verbatim and aborted the install, leaving the +// fully-extracted files stranded in the temp directory while nvm reported +// success — the root cause of issue #1357. func Rename(old, new string) error { - old_drive := filepath.VolumeName(old) - new_drive := filepath.VolumeName(new) - - if old_drive == new_drive { - return os.Rename(old, new) + sameVolume := filepath.VolumeName(old) == filepath.VolumeName(new) + + // Fast path: a real rename is only possible within a single volume. + if sameVolume { + var err error + for _, backoff := range []time.Duration{0, 1, 2, 4} { + if backoff > 0 { + time.Sleep(backoff * time.Second) + } + if err = os.Rename(old, new); err == nil { + return nil + } + } + // The rename kept failing even though both paths are on the same + // volume. Fall through to copy + remove rather than aborting. } + return copyThenRemove(old, new) +} + +// copyThenRemove recursively copies old to new and then deletes old. It is used +// both for cross-volume moves and as a fallback when an in-volume rename fails. +func copyThenRemove(old, new string) error { // Get file or directory info info, err := os.Stat(old) if err != nil {