Skip to content

fix(linker): reclaim an incumbent node_modules tree on Windows - #613

Merged
colinhacks merged 1 commit into
mainfrom
win-npm-takeover
Jul 29, 2026
Merged

fix(linker): reclaim an incumbent node_modules tree on Windows#613
colinhacks merged 1 commit into
mainfrom
win-npm-takeover

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

nub install aborted on Windows in any project whose node_modules npm or yarn had already written:

failed to link node_modules
I/O error at D:\proj\node_modules\express: Access is denied. (os error 5)

reconcile_top_level_link clears a stale top-level entry with remove_dir falling back to remove_file. Those cover a junction and a plain file, but an incumbent package manager leaves a populated real directory, which is neither — remove_dir answers ERROR_DIR_NOT_EMPTY, remove_file then answers ERROR_ACCESS_DENIED. That is os 5, which is_transient_fs_error counts as transient, so it also burns the full ~10s retry ladder first.

Reclaiming that tree is the linker's job, and the Unix branch has always recursed for it — which is why this is Windows-only. The recursion is gated on is_real_dir so it reaches that shape and never a junction, whose target must survive, and it sits inside the existing ladder rather than adding a second.

Reproduced on 0.6.0 and canary on a windows-latest runner.

Note the incumbent-tree test exercises the cfg(not(windows)) branch on Unix, where it passes either way; the windows leg of aube-parity is what makes it meaningful. The test says so in place.

`nub install` in a project whose `node_modules` npm or yarn had already
written aborted on Windows at the first hoisted dependency:

    failed to link node_modules
    I/O error at D:\proj\node_modules\express: Access is denied. (os error 5)

`reconcile_top_level_link` clears a stale top-level entry with `remove_dir`
falling back to `remove_file`. Those cover a junction and a plain file, but
an incumbent package manager leaves a POPULATED REAL directory, which is
neither: `remove_dir` answers ERROR_DIR_NOT_EMPTY and `remove_file` then
answers ERROR_ACCESS_DENIED. That code is os 5, which
`is_transient_fs_error` counts as transient, so the removal also burns the
full ~10s retry ladder before failing.

Reclaiming that tree is the linker's job — this caller owns the entry, and
the Unix branch has always recursed for it, which is why the failure is
Windows-only. The recursive delete is gated on `is_real_dir` so it reaches
exactly that shape and never a junction, whose target must survive, and it
sits inside the existing retry ladder rather than adding a second one.

Reproduced on 0.6.0 and canary on a windows-latest runner. Note the new
`reconcile_top_level_link_reclaims_an_incumbent_package_manager_tree` test
exercises the `cfg(not(windows))` branch on Unix, where it passes either
way — the windows leg of `aube-parity` is what makes it meaningful, and the
test says so in place.
Copilot AI review requested due to automatic review settings July 29, 2026 16:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview, Comment Jul 29, 2026 4:29pm

Request Review

@pullfrog pullfrog Bot left a comment

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.

ℹ️ The fix itself holds up under scrutiny — two rough edges are documentation-level.

Reviewed changes — a Windows-only reclaim path in the vendored aube linker so an install over a node_modules tree an incumbent npm or yarn wrote no longer burns the transient-retry ladder and aborts.

  • Recurse into a populated real directory on Windowsreconcile_top_level_link gains a third branch inside the existing ladder: when remove_dir fails and aube_util::fs::is_real_dir holds, clear the slot with remove_dir_all. The cfg(not(windows)) arm has always recursed, so behavior off Windows is unchanged.
  • Widen reconcile_top_level_link to pub(crate) — lets the sibling test module call it directly instead of driving it through link_all.
  • Add two linker regression tests — one for the incumbent-tree reclaim, one asserting a stale junction is unlinked while its target survives.

Three things I checked and found sound, recorded so they don't get re-litigated:

  • Every one of the seven call sites builds link_path as <node_modules-or-hidden-hoist-root>.join(name), so the recursive delete can never reach a workspace source dir or the shared store — those only ever arrive as the second argument.
  • The recursion cannot follow a reparse point for two independent reasons: RemoveDirectoryW removes a junction regardless of emptiness (so a junction never reaches the gate), and is_real_dir is false whenever read_link succeeds. remove_dir_all has also declined to follow reparse points since 1.58.1.
  • The new tests genuinely run on the target platform — mod tests is #[cfg(test)] only, neither test carries a platform cfg, and the aube-parity-windows job (cargo test --workspace in vendor/aube) gates pull requests on any vendor/aube/** edit. Both also pass locally on Linux.

ℹ️ Nitpicks

  • The function's doc comment still describes the removal as "the same remove_dir().or_else(remove_file()) fallback ... to unlink both shapes" (vendor/aube/crates/aube-linker/src/link.rs:1845, just above the diff). The Windows arm now handles three.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Err(e)
}
})
.or_else(|_| std::fs::remove_file(link_path))

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.

ℹ️ The trailing fallback now also catches a failed remove_dir_all, and remove_file against a directory always answers os 5 — so when the recursive reclaim is the thing that fails (a dev server holding a file inside the incumbent tree), the install reports a bare Access is denied rather than the sharing violation that actually caused it. Retry classification is unaffected, since 5 and 32 are both transient.

Technical details
# Trailing `remove_file` masks the `remove_dir_all` error

## Affected sites
- `vendor/aube/crates/aube-linker/src/link.rs:1941``.or_else(|_| std::fs::remove_file(link_path))` discards the error from the new recursive branch. `remove_file` can never succeed on a path `is_real_dir` just classified as a directory, so on that branch it only rewrites the error.

## Required outcome
- When the recursive reclaim fails, the error the ladder classifies and the install ultimately reports is the one `remove_dir_all` produced, not os 5 from a `remove_file` that had no chance of succeeding.

## Suggested approach
Make the two fallbacks alternatives rather than a chain, so each shape's own error survives:

```rust
std::fs::remove_dir(link_path).or_else(|_| {
    if aube_util::fs::is_real_dir(link_path) {
        std::fs::remove_dir_all(link_path)
    } else {
        std::fs::remove_file(link_path)
    }
})
```

## Open questions for the human
- The current laundering buys accidental retry coverage: a non-transient `remove_dir_all` failure (e.g. `ERROR_DIR_NOT_EMPTY` from something repopulating the tree mid-delete) becomes os 5 and gets retried, where the shape above would fail immediately. Worth keeping that, at the cost of the causal error?

Comment on lines +2387 to +2389
// A junction is the other shape reaching that removal, and its TARGET must
// survive being unlinked — the recursion is gated on `is_real_dir` precisely
// so it cannot follow one.

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.

ℹ️ This test passes with or without the is_real_dir gate: RemoveDirectoryW removes a junction regardless of emptiness, so remove_dir succeeds on the line above the gate and the recursive branch is never entered (on Unix the remove_dir/remove_file pair handles it). It's a real guard against reordering the chain, but the comment reads as though it demonstrates the gate — worth pinning what it actually proves, in the same spirit as the NOTE on the test above it.

@colinhacks
colinhacks merged commit bac8e7b into main Jul 29, 2026
53 checks passed
colinhacks added a commit that referenced this pull request Jul 29, 2026
Picks up #612, #609 and #613, which landed while this bump was in flight and
which the PR body flagged as overlapping.

One conflict, in commands/outdated.rs: #609 added a HashSet import and still
called max_satisfying_version, which aube v1.35.0 renames to wanted_version.
Unions the import and keeps the new name. The argument stays unreferenced —
packument binds to &Packument out of the Some(Ok(p)) arm.
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0

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.

2 participants