Fix/1357 silent extraction failure - #1362
Open
fschaupp wants to merge 2 commits into
Open
Conversation
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 <[email protected]>
…#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 <[email protected]>
|
This PR is stale because it has been open 45 days with no activity. |
|
bump |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1357
Two fixes for nvm install reporting success while leaving you with no usable version.
Stop masking failures. The extract/move path swallowed errors, so a failed install still printed "Complete" and left an empty version dir. Failures are now surfaced loudly and the partial install is rolled back.
Fix the actual move (the real cause). Installs extract Node into a temp dir, then move it into the nvm root via utility.Rename. That only fell back to copy+delete across different volumes; on the common same-volume case it did a bare os.Rename, which on Windows fails when a freshly-extracted file is briefly
locked (e.g. antivirus scanning node.exe). The files were left stranded in %TEMP%\nvm-install-* while nvm reported success. Rename now retries with a short backoff and falls back to copy+delete for every move — matching the manual workaround users found in the issue (copying the extracted folder into the nvm
root).