fix(linker): reclaim a per-project virtual store when relinking on Windows - #595
Conversation
…ndows `nub add` / `nub remove` on an existing project aborted on Windows with `failed to link node_modules` -> `Cannot create a file when that file already exists. (os error 183)`, leaving the tree unlinked. Two defects compose. The real-directory test was Unix-only. Several sites read "this entry is a plain directory, not a link" off `read_link` failing with `InvalidInput` -- the errno a Unix readlink(2) gives for a non-link. Windows fails with ERROR_NOT_A_REPARSE_POINT (4390), which std's error table has no entry for, so it arrives as `Uncategorized`. Every such site answered "not a real directory" for every real directory there. `detect_aube_dir_gvs_mode` is the one that matters: it could never return `Some(false)` on Windows, so `reset_on_mode_change` never wiped a per-project tree, and any install that had run with the global virtual store off left populated directories in the slots the next install wanted to link. The stale-entry cleanup was non-recursive. Those directories classified as `Stale` correctly, then `remove_dir` / `remove_file` failed to clear them, both errors were discarded, and the junction creation that followed aborted the install. The git/url sibling path already used `try_remove_entry` for exactly this; the registry path never got it. `aube_util::fs::is_real_dir` now carries the platform split in one place, keyed on whether the entry is a link at all rather than on a failed `read_link`'s error kind. That spelling also keeps reparse points that are not links -- OneDrive cloud placeholders, dedup stubs -- classified as the real directories they are. `create_dir_link` clears a leftover link and retries once, but stays non-recursive so a populated directory still surfaces rather than being wiped by a generic helper. Neither defect was reachable by CI: the aube suite ran on ubuntu only, and `all_real_dirs_reads_as_per_project` -- the test that catches this -- was itself `#[cfg(all(test, unix))]`. The gvs-detect tests now build their entries through `create_dir_link` and compile everywhere, and aube-parity gains a `cargo test --workspace` leg on windows-latest that gates pull requests. It is path-filtered to vendor/aube/**, so it costs nothing on a PR that does not touch the vendored tree. Closes #566 Closes #576
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
ℹ️ No functional issues found — two rationale comments state the inverse of std's actual Windows behavior.
Reviewed changes — a Windows-only install abort (os error 183) where the "is this a real directory" test was silently Unix-only, plus the cleanup and CI gaps that let it reach users.
- Replace the
InvalidInputreal-dir test withaube_util::fs::is_real_dir— the old spelling keyed off the errno Unixreadlink(2)returns for a non-link; Windows returnsERROR_NOT_A_REPARSE_POINT(4390), which std leavesUncategorized, sodetect_aube_dir_gvs_modecould never returnSome(false)there. Both call sites converted; I greppedvendor/aubeandcrates/and found no remaining site on the old idiom. - Make the
Stale-branch cleanup recursive —try_remove_entryreplaces a non-recursiveremove_dir().or_else(remove_file)that could not clear a populated per-project tree, and now routes throughremove_dir_all_with_retryfor the Windows sharing-violation backoff. - Add a clear-and-retry to
create_dir_link— anAlreadyExistsfrom the junction/symlink create clears a leftover link or empty directory and retries once, while a populated directory still surfaces the error rather than being wiped by a generic helper. - Close the coverage gap that hid the bug —
gvs_mode_detect_testsmoves off#[cfg(all(test, unix))]and builds every entry throughcreate_dir_link, and a new PR-gatingaube-parity-windowsjob runs aube's suite onwindows-latest.
ℹ️ The new Windows parity leg gates PRs from the moment it merges
aube-parity-windows is the first time aube's cargo test --workspace runs on Windows with nub's fork delta applied, and it is wired to gate every pull request that touches vendor/aube/**. Upstream jdx/aube runs the same invocation on a Windows leg, so the upstream portion of the tree is de-risked — the untested surface is specifically the nub-only delta.
The consequence is that a fork-only Windows failure discovered later blocks unrelated vendor/aube work, with no stated escape hatch:
- the job has no
if:guard and nocontinue-on-error, unlikeaube-conformance-windows, which is deliberately gated offpull_request - the workflow's own doctrine ("everything in
vendor/aubestays conceivably upstreamable") is what makes fork-only failures expected to be rare, not impossible
Technical details
# The new Windows parity leg gates PRs from the moment it merges
## Affected sites
- `.github/workflows/aube-parity.yml:88-120` — new `aube-parity-windows` job, `runs-on: windows-latest`, no `if:` guard, PR-gating by omission
- `.github/workflows/aube-parity.yml:179-184` — the sibling `aube-conformance-windows` job, which takes the opposite call (`if: github.event_name != 'pull_request'`) with a stated rationale
## Required outcome
- Confirm this PR's own `aube-parity-windows` run is green before merge — that run is the only existing evidence the nub fork delta passes on Windows.
- Decide and record what happens when a later `vendor/aube` PR trips a Windows-only failure it did not cause: fix-forward, temporary `continue-on-error`, or move the leg to `push` + `schedule` like the conformance leg.
## Open questions for the human
- Is PR-gating the intended long-term state, or a bootstrap posture until the Windows leg has a track record?ℹ️ Nitpicks
- The new comments reference issues as bare
nub#566/nub#576(aube-util/src/fs.rs:17,install/settings.rs:203,install/settings.rs:1291,aube-linker/src/tests.rs:2230). The existing convention insidevendor/aubeis the fully-qualified form that resolves as a GitHub cross-reference —nubjs/nub#441(aube-lockfile/src/drift.rs:521,aube/src/state.rs:402),nubjs/nub#492(aube-lockfile/tests/no_churn_write_guard.rs:222).
Claude Opus | 𝕏
| // directory is reused as cached. Neither | ||
| // `file_type().is_symlink()` nor a `read_link` error | ||
| // kind classifies that correctly on both platforms — | ||
| // `aube_util::fs::is_real_dir` carries the split. |
There was a problem hiding this comment.
Only the read_link-error-kind half of this was ever broken. Under symlink_metadata, std's Windows FileType::is_symlink() tests the reparse-tag name-surrogate bit 0x20000000, which IO_REPARSE_TAG_MOUNT_POINT (0xA0000003) sets — so a junction reports is_symlink() == true and is_dir() == false, and file_type() would in fact have classified this correctly on both platforms.
Technical details
# Rationale comments state the inverse of std's Windows `FileType` behavior
## Affected sites
- `vendor/aube/crates/aube-linker/src/link.rs:337-340` — "Neither `file_type().is_symlink()` nor a `read_link` error kind classifies that correctly on both platforms"; the `is_symlink()` half is false.
- `vendor/aube/crates/aube-linker/src/tests.rs:2277-2279` — "a junction reports `is_dir() == true` / `is_symlink() == false`, which is what defeats the obvious file-type test"; both halves are inverted.
- `vendor/aube/crates/aube-linker/src/link.rs` ~L1794 (pre-existing, outside this diff) — same claim, and the likely origin of it.
- `vendor/aube/crates/aube-linker/src/sweep.rs:110-124` — `remove_hidden_hoist_tree` depends on the CORRECT behavior: its `is_symlink()` branch is what keeps `remove_dir_all` from being handed a junction. With the claim above in the tree, these two are contradictory and a future reader will "fix" the wrong one.
## Required outcome
- The comments should describe the actual platform split: the error KIND of a failed `read_link` differs per platform (Unix `EINVAL` → `InvalidInput`; Windows `ERROR_NOT_A_REPARSE_POINT` 4390 → `Uncategorized`, since `decode_error_kind` has no entry for it). `file_type()` is not the thing that was broken.
- `is_real_dir` itself needs no change — it is correct under either reading, and `symlink_metadata().is_dir()` returning `false` for a junction means the `read_link` pre-check is belt-and-braces rather than load-bearing. Say that, or drop the claim, but do not leave the tree asserting both.
## Suggested approach
std 1.93.0, `library/std/src/sys/fs/windows.rs`:
```rust
pub fn is_symlink(&self) -> bool {
self.is_reparse_point() && self.is_reparse_tag_name_surrogate()
}
pub fn is_dir(&self) -> bool {
!self.is_symlink() && self.is_directory()
}
fn is_reparse_tag_name_surrogate(&self) -> bool {
self.reparse_tag & 0x20000000 != 0
}
```
Per MS-FSCC 2.1.2.1, bit `0x20000000` is the name-surrogate bit, set by both `IO_REPARSE_TAG_SYMLINK` (`0xA000000C`) and `IO_REPARSE_TAG_MOUNT_POINT` (`0xA0000003`).| // directory is reused as cached. Neither | |
| // `file_type().is_symlink()` nor a `read_link` error | |
| // kind classifies that correctly on both platforms — | |
| // `aube_util::fs::is_real_dir` carries the split. | |
| // directory is reused as cached. The platform split | |
| // is in the error KIND of a failed `read_link`; | |
| // `aube_util::fs::is_real_dir` carries it. |
| // Unix symlink on Unix and an NTFS junction on Windows, and a junction | ||
| // reports `is_dir() == true` / `is_symlink() == false`, which is what | ||
| // defeats the obvious file-type test. |
There was a problem hiding this comment.
Inverted: a junction reports is_symlink() == true and is_dir() == false under symlink_metadata, so the file-type test is not what was defeated here — the error kind of a failed read_link is. See the details on the link.rs comment for the std source and the contradicting assumption in sweep.rs::remove_hidden_hoist_tree. The test itself is a good one; only its premise needs restating.
The new windows-latest aube leg surfaced a pre-existing isolation bug:
`pnpm_config_dir_with(home, xdg)` discards `home` on Windows and returns
the real `%LOCALAPPDATA%\pnpm\config`, so these three write their
`auth.ini` fixture into the actual user profile. Siblings then read it
and three unrelated auth tests resolve `Some("auth-ini-token")`; on a
developer machine it would overwrite real pnpm credentials.
Ignored on Windows only -- they still run everywhere else -- so the new
leg gates on a real signal instead of a known-red one. The per-OS branch
needs to become injectable; tracked in #605, and removing these
attributes is the definition of done there.
Refs #605
There was a problem hiding this comment.
ℹ️ No functional issues in the new commit — the quarantine is accurate and complete, but it is a guardrail where the root fix is a parameter.
Reviewed changes — one new commit, which quarantines the three auth.ini fixture tests that the new windows-latest leg would otherwise run non-hermetically.
- Skip three
pnpm_global_auth_ini_*tests on Windows —#[cfg_attr(windows, ignore)]plus a rationale comment, becausepnpm_config_dir_with(Some(tmp_home), None)discards its injected home on Windows and resolves the real%LOCALAPPDATA%\pnpm\config, so the fixture escapes its tempdir into the user profile. Tracked as #605.
The quarantine set checks out. Only those three write through the escaping helper; the XDG-override sibling is correctly left running (it writes an explicit path and hits the helper's XDG short-circuit, which is hermetic everywhere), and the three siblings #605 names only read the leftover, so a clean runner is unaffected. I also audited every other env-direct path helper in vendor/aube/crates — aube-store/src/dirs.rs, aube-util/src/adaptive.rs, aube-resolver/src/primer.rs, aube-runtime/src/paths.rs, aube/src/tool_shims.rs — and found no other test that reaches the real, non-injectable form to write.
Claude Opus | 𝕏
| // tests deliberately exercise the per-OS branch, so the fix is to make that | ||
| // branch injectable rather than to reroute the tests. Tracked in nub#605. | ||
| #[test] | ||
| #[cfg_attr(windows, ignore = "escapes its tempdir on Windows; nub#605")] |
There was a problem hiding this comment.
ℹ️ This is a guardrail where the root fix is a parameter: the Windows branch of pnpm_config_dir_with reads LOCALAPPDATA env-direct instead of accepting it, which the comment above already identifies as the thing to fix. AGENTS.md's testing doctrine names #[ignore] as banned for a non-hermetic test and prefers the root-cause fix over a mitigation regardless of effort. ignore is also a soft gate — cargo test -- --include-ignored and nextest's --run-ignored all still overwrite a real developer's auth.ini, the outcome the comment calls out.
Technical details
# The auth.ini quarantine mitigates rather than fixes a helper that discards its injected root
## Affected sites
- `vendor/aube/crates/aube-util/src/env.rs:272-277` — the Windows branch of `pnpm_config_dir_with` ignores the `home` parameter and calls `local_app_data()`, which reads `LOCALAPPDATA` from the process environment. `LOCALAPPDATA` is the only input to this function that is not injectable, and the doc comment on `pnpm_config_dir` states the `_with` form exists so "tests can pin a tempdir without mutating process-wide env."
- `vendor/aube/crates/aube-registry/src/config/tests.rs:2518, 2602, 2636` — the three `#[cfg_attr(windows, ignore)]` attributes added by this commit.
- `vendor/aube/crates/aube-registry/src/config/load.rs:996` — `pnpm_global_auth_ini_path`'s call site; whatever signature the helper grows has to thread through here too.
## Required outcome
- A Windows test can pin `pnpm_config_dir_with`'s per-OS branch to a tempdir, so the three tests run on the new `aube-parity-windows` leg instead of being skipped on the platform whose behavior they exist to assert.
- No test can write to a real user profile path even when run with `--include-ignored`.
## Suggested approach
Make `LOCALAPPDATA` the third injected input rather than an env read, mirroring `home` / `xdg_config_home`:
```rust
pub fn pnpm_config_dir_with(
home: Option<&Path>,
xdg_config_home: Option<&Path>,
local_app_data: Option<&Path>,
) -> Option<PathBuf>
```
`pnpm_config_dir()` passes `local_app_data().as_deref()`; the three tests pass `Some(tmp_home.join("AppData/Local"))` and drop the `cfg_attr`. `resolves_per_os_config_dir_without_xdg` in `env.rs:311` can then assert the Windows branch exactly instead of accepting either shape.
If the parameter churn is unwanted, `#[cfg(not(windows))]` is at least a hard gate rather than a soft one — but that still leaves the platform branch untested on the platform it is for, which is the same shape of gap this PR's `gvs_mode_detect_tests` change exists to close.
## Open questions for the human
- Is landing the injectable helper inside this PR acceptable, or is deferring to nubjs/nub#605 a deliberate sequencing call to keep the linker fix reviewable?The real Windows CI leg disagreed with the assumption this was built on. `recreating_a_junction_at_a_different_target_still_fails` asserted that re-pointing an existing junction at a DIFFERENT target still errors; on a real windows-latest runner `create_dir_link` returned `Ok` instead. That matters more than the failing assertion. `junction::create` calls `fs::create_dir(junction)` (junction-2.0.0, internals.rs:37), so 183 on an existing junction is exactly what the code expected, and the guard should have rejected a mismatched target and propagated the error. It did not, and I cannot explain why from here — which means the tolerance was accepting cases I do not understand, on a platform I cannot reproduce on. An arm that silently accepts a junction pointing at the WRONG target would leave a package resolving to another package's tree. Nothing is lost by removing it. #595 fixed the actual cause of the reported `os error 183` — a populated real directory cleared with a non-recursive `remove_dir`, so junction creation aborted — and closed #566 and #576. This arm was speculative hardening for a concurrent-writer collision I never demonstrated, layered on top of an already-fixed bug. The rest of the branch stands: it is verified by a control on any platform, and none of it depends on this. Refs #552
…he store (#598) * fix(linker): stage every materialize so a failed link cannot poison the store A link that failed partway left a half-populated `.store/<dep_path>` behind, and the next run's `if aube_entry.exists() { cached }` gate reported that package as already linked. The tree never converged: every later install said "Already up to date" over a `node_modules` that could not resolve, and `rm -rf node_modules` was the only way out. Five call sites in link.rs passed `base_dir == final dir` to `materialize_into`, which documents that it "always writes into a fresh location" and therefore skips both the defensive `remove_file(dst)` per file and the existence check before `create_dir_link`. Those sites also had no cleanup on error, unlike the staged callers which `remove_dir_all` their tmp base. All five now go through `ensure_in_aube_dir`, which stages into `.tmp-<pid>-<id>` and atomic-renames into place, so a failure leaves only a tmp dir that `sweep_stale_tmp_dirs` reclaims. The two par_iter sites keep their outer `exists()` fast path so a warm run still skips `load_index`. Separately, the per-file link was the one operation with no transient-error retry while the junction and directory-removal paths had carried one all along. On Windows a single `ERROR_SHARING_VIOLATION` from another process holding a handle — Defender mid-scan, the search indexer, a watcher — aborted the entire install. The terminal copy in `link_file_fresh` now retries on the shared ladder; `hard_link`/`reflink` deliberately do not, since both already fall through to that copy. `is_transient_rename_error` gets the same treatment for a subtler reason: Rust maps `ERROR_SHARING_VIOLATION` to no `ErrorKind` at all (it decodes to `Uncategorized`), so an `ErrorKind`-only predicate is silently blind to it and the rename retry had never once fired for os 32. The raw-code arm is gated to Windows because raw 32 is EPIPE on Unix. The same blindness made `remove_dir_all_with_retry`'s `ErrorKind::Other` arm dead code; it is gone, and all three paths now share one predicate. Two fixes the staging change made necessary. `link_workspace` never swept orphan `.tmp-*` dirs the way `link_all` does, so its staged materializations would have leaked one per aborted install. And the GVS disk-materialize site dropped a stale store symlink best-effort before writing; because `ensure_in_aube_dir` opens with an `exists()` gate and `exists()` follows symlinks, a surviving link would have read as "already materialized" and silently skipped the ejection, leaving a store symlink where a real project-local directory is required. That removal is now checked. Prevention only: an already-corrupted tree still needs one `rm -rf node_modules`, because the gate continues to trust an entry that exists. Closes #552 Refs #566 Refs #576 * fix(linker): retry the remaining transient-fatal paths; accept a correct junction Four more instances of the class #552 exposed, found by auditing the paths the first commit did not touch. `reconcile_top_level_link` removed a stale `node_modules/<name>` entry without retry, and its failure was fatal. That entry is the likeliest thing in the tree to be held open — it is what a dev server, a `--watch` task, or an editor has its handle on — so a reinstall aborted over a lock that would have cleared in milliseconds. Creating a junction failed on `ERROR_ALREADY_EXISTS` even when the junction already pointed at the target we were about to point it at. Callers reach that call after their own stale-entry reconciliation, so a surviving correct entry means a concurrent writer got there first — the outcome we wanted. That is the `os error 183` an incremental `add` surfaced. A junction pointing somewhere else is still an error; the two are now distinguished by comparing both sides through `canonicalize`, so a short name, a case difference, or a one-sided `\\?\` prefix does not read as a mismatch. `try_remove_entry` ran a raw `remove_dir_all` and discarded the result. A wipe that aborts partway leaves files behind, and the refill that follows only writes paths present in the NEW index — anything the old version had and the new one does not survives and stays resolvable, silently blending two package versions. This one produces a wrong tree rather than an error, which makes it the worst of the four. `mkdirp` and the hoisted per-file parent creation were unretried against a slot in Windows' pending-delete state, which is reachable precisely because a wipe just ran against a path something else holds open. `mkdirp` now calls `std::fs::create_dir_all` directly rather than `xx::file::mkdirp`, because `xx` wraps the error in its own type and drops `raw_os_error()` — the retry predicate matches on the raw code, so wrapping the `xx` call would have compiled, read correctly, and never once retried. The `xx` helper is an `exists()` check plus the same call, and `create_dir_all` already no-ops on an existing directory, so nothing else changes. Windows compilation of all of this was verified by cross-checking against `x86_64-pc-windows-gnu` with mingw; the msvc target cannot be checked here because `zstd-sys` needs MSVC headers. Refs #552 Refs #566 Refs #576 * fix(linker): bound the retry ladder on best-effort sweeps The retry added to `try_remove_entry` in the previous commit used the full ~10s ladder, but that function runs once per entry inside three sweep loops. The cost there is not 10s, it is 10s PER LOCKED ENTRY — a branch switch with a dev server running would have turned a cleanup pass into a multi-minute stall. Split the ladder: operations whose failure is fatal to the install keep 10 attempts, best-effort removals the caller proceeds past either way get 4 (~750ms). The transient being waited out is an AV scan window, which clears well inside a second, so the short ladder catches essentially everything the long one would. `sweep_stale_tmp_dirs` keeps the full ladder deliberately: its 10-attempt behavior predates this branch, and the orphans it reclaims come from dead processes, so nothing is holding them open. Refs #552 * revert(linker): drop the os-183 junction tolerance as unproven The real Windows CI leg disagreed with the assumption this was built on. `recreating_a_junction_at_a_different_target_still_fails` asserted that re-pointing an existing junction at a DIFFERENT target still errors; on a real windows-latest runner `create_dir_link` returned `Ok` instead. That matters more than the failing assertion. `junction::create` calls `fs::create_dir(junction)` (junction-2.0.0, internals.rs:37), so 183 on an existing junction is exactly what the code expected, and the guard should have rejected a mismatched target and propagated the error. It did not, and I cannot explain why from here — which means the tolerance was accepting cases I do not understand, on a platform I cannot reproduce on. An arm that silently accepts a junction pointing at the WRONG target would leave a package resolving to another package's tree. Nothing is lost by removing it. #595 fixed the actual cause of the reported `os error 183` — a populated real directory cleared with a non-recursive `remove_dir`, so junction creation aborted — and closed #566 and #576. This arm was speculative hardening for a concurrent-writer collision I never demonstrated, layered on top of an already-fixed bug. The rest of the branch stands: it is verified by a control on any platform, and none of it depends on this. Refs #552
`link_all` and `link_workspace` carry near-duplicate step-1 GVS-populate loops. #595 fixed the `EntryState::Stale` removal in the first — a non-recursive `remove_dir` cannot clear a POPULATED real directory, the swallowed error resurfaces as EEXIST / os-183 from the link creation right below it — and left the identical arm in the workspace loop untouched. So a workspace wedged permanently where a single-package project self-healed: × failed to link workspace node_modules ╰─▶ I/O error at C:\ws\node_modules/.store\express@4.18.2: Cannot create a file when that file already exists. (os error 183) `nub install`, `nub install --force`, and member-level `nub add` / `nub remove` all failed the same way; only deleting node_modules recovered. Confirmed on a Windows Server 2022 box against 0.6.0-canary.20260729.129 with a single-package control that recovered on the same perturbation. Not Windows-specific despite where it was found: with this arm reverted the new test fails on macOS too, with EEXIST (os 17). The trigger is a MIXED store — some entries links, one a real directory. `detect_aube_dir_gvs_mode` returns `Some(true)` on the first link it sees, so the mode-change wipe does not fire and the survivor reaches this arm. That wipe is currently the only thing pre-empting the failure, and it is a different layer, so treat this as latent rather than unreachable: a partial cache restore, a crash, or external tooling all produce the same shape.
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |

nub add/nub removeaborted on Windows withos error 183, leaving the tree unlinked.read_linkfailing withInvalidInputwas the "is a real directory" test. That errno is Unix-only, sodetect_aube_dir_gvs_modenever returnedSome(false)on Windows andreset_on_mode_changenever wiped a per-project tree. The surviving directories were cleared with a non-recursiveremove_dir, so junction creation aborted.is_real_dirnow keys on whether the entry is a link; cleanup usestry_remove_entry, as the git/url path already did (#341).aube's suite ran ubuntu-only and the covering test was
unix-gated. Both fixed, plus a PR-gating windows-latest leg.Closes #566
Closes #576