fix(windows): retry the post-extraction rename blocked by A/V handles - #1585
Open
sacru2red wants to merge 1 commit into
Open
fix(windows): retry the post-extraction rename blocked by A/V handles#1585sacru2red wants to merge 1 commit into
sacru2red wants to merge 1 commit into
Conversation
`fnm install` on Windows could fail with "Can't download the requested binary: Access is denied. (os error 5)" long after the download and the extraction had both succeeded. Windows denies a directory rename while any file inside the tree has an open handle that does not grant FILE_SHARE_DELETE, which is what a real-time A/V scanner holds on the freshly extracted node.exe. Retry both post-extraction renames with bounded backoff, and stop reporting post-download filesystem failures as download errors: - add fs_retry::rename_with_retry, backing off 10ms at a time up to 100ms within a budget from FNM_RENAME_RETRY_TIMEOUT_MS (default 5000, 0 disables). is_transient_lock is cfg(windows) and matches ERROR_ACCESS_DENIED, ERROR_SHARING_VIOLATION and ERROR_LOCK_VIOLATION; elsewhere it is always false, so behaviour is unchanged. - use it in downloader::install_node_dist and DirectoryPortal::teleport. - add downloader::Error::CantMoveIntoPlace for the three post-download filesystem steps and forward it transparently through commands::install instead of wrapping it in DownloadError. The new tests pin the condition without an A/V product installed by opening node.exe with share_mode(FILE_SHARE_READ). Refs Schniz#1583, Schniz#1193 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
What actually fails
On Windows,
fnm installcan fail withCan't download the requested binary: Access is denied. (os error 5)even though the download and the extraction both succeeded. The failure is one of the twostd::fs::renamecalls that run after extraction —<tmp>\node-vX.Y.Z-win-x64→<tmp>\installation, andDirectoryPortal::teleport. Windows denies a directory rename while any file inside the tree has an open handle that does not grantFILE_SHARE_DELETE, which is exactly the handle a real-time A/V scanner holds on the freshly writtennode.exe. Two tells are present in every report: the progress bar reaches 100%, and the message is notCan't extract the file:.Full root-cause analysis, elimination of the usual suspects (path ACLs, elevation, folder redirection, Controlled Folder Access, proxy/TLS inspection, fnm version), and a standalone repro are in #1583. It also explains why the workarounds in #1193 contradict each other —
pwsh -noprofile, a reboot,wsl --shutdown, "it worked the second time" all reduce to releasing or avoiding a handle.What this PR changes
1. The error no longer says "download".
downloader::Errorgains aCantMoveIntoPlacevariant, and the three post-download filesystem steps (read_dir, the rename,teleport) map into it instead of falling through the blanket#[error(transparent)] IoError.commands::installforwards it transparently rather than wrapping it inCan't download the requested binary.create_dir_allbefore the download keeps the old variant, so "failed before the download" and "failed after it" stay distinguishable.Before:
error: Can't download the requested binary: Access is denied. (os error 5)After:
error: Can't move the extracted files into place: Access is denied. (os error 5)2. Both renames are retried with bounded backoff (
src/fs_retry.rs). Backoff grows 10ms at a time up to 100ms, within a budget read fromFNM_RENAME_RETRY_TIMEOUT_MS(default5000,0disables retrying).is_transient_lockis#[cfg(windows)]and matchesERROR_ACCESS_DENIED (5),ERROR_SHARING_VIOLATION (32),ERROR_LOCK_VIOLATION (33); on every other platform it is aconst false, so behaviour there is unchanged — a singlestd::fs::rename, no sleep, no extra syscall.FNM_RENAME_RETRY_TIMEOUT_MSis read directly rather than declared onFnmConfiglike the otherFNM_*variables, to keep this an escape hatch rather than a documented knob — it adds no flag to--helpand leavesdocs/commands.mduntouched. Happy to move it intoFnmConfig(hidden or not) if you'd rather have it consistent with the rest.This is the same shape as
graceful-fs, which npm itself depends on. Itspolyfills.jsretriesrenameonEACCES/EPERM/EBUSYfor up to 60s, with this comment:Why retry rather than a fixed sleep: no constant is correct, because scan time scales with file size, CPU and load, and a fixed delay taxes every healthy install. Retrying costs nothing where nothing interferes — the first attempt succeeds.
Tests
src/fs_retry.rsreproduces the condition deterministically, without an A/V product installed.retries_until_the_handle_is_releasedopensnode.exewithshare_mode(FILE_SHARE_READ)— deliberately withholdingFILE_SHARE_DELETE— releases it from another thread after 300ms, and asserts the rename of the parent directory eventually succeeds.does_not_retry_when_the_budget_is_zeroasserts a zero budget returns the original OS error immediately. Both are#[cfg(all(test, windows))]; a third, cross-platform test asserts an unblocked rename never touches the retry path.No e2e test: holding a scanner-like handle on
node.exeduring the window between extraction and rename is inherently racy from the shell harness, so the condition is pinned at the unit level instead.Trade-off
Retrying
ERROR_ACCESS_DENIEDdelays a genuine permission error by up to the budget. Mitigated by gating on Windows only, a 5s default, returning the original error unchanged once the budget expires, andFNM_RENAME_RETRY_TIMEOUT_MS=0to opt out. The escape hatch matters because affected users often cannot change their endpoint agent's policy — on corporate SEP deployments the exclusion UI is greyed out and managed centrally, so "add an A/V exclusion" means filing a security ticket and waiting.Fixes #1583
Refs #1193