fix(linker): stage every materialize so a failed link cannot poison the store - #598
Conversation
…he 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
There was a problem hiding this comment.
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No correctness issues found in the staging swap — two robustness rough edges inline, plus a coverage note below.
Reviewed changes — the full 4-file diff at 0b816d9a, which makes every per-project .aube/<dep_path> materialization atomic and teaches the Windows filesystem paths to retry sharing violations.
- Route five direct materializations through
ensure_in_aube_dir—link.rsno longer callsmaterialize_into(&aube_dir, &aube_dir, …)anywhere; all five sites now stage into.tmp-<pid>-<id>/and atomic-rename, so a mid-link failure leaves a sweepable tmp dir instead of a half-populated entry that the next run'sexists()gate reports as complete. - Add a shared Windows transient-retry ladder —
with_transient_retryplusis_transient_fs_error(raw os 5/32) insweep.rs;remove_dir_all_with_retryis refactored onto it and Unix stays a straight passthrough. - Make
is_transient_rename_errorseeERROR_SHARING_VIOLATION— os 32 decodes toUncategorized, so theErrorKind-onlymatches!had never matched the most common Windows transient the rename hits. - Retry the per-file byte transfer — every
std::fs::copyinlink_file_fresh(macOS small-file, reflink fallback, hardlink fallback,LinkStrategy::Copy) now goes throughcopy_through_transients. - Sweep stranded tmp dirs in
link_workspace— the sweeplink_allalready ran was missing on the workspace path, so aborted workspace installs leaked one tmp dir each. - Check the stale shared-store link removal on the GVS eject path — a surviving symlink whose target is live would have read as "already materialized" and silently skipped the ejection; that now fails loudly.
- Add a regression test and two predicate tests — the regression test removes a CAS shard so
link_allfails mid-materialize, then asserts no.aubeentry survives.
I traced the five swapped call sites against their pre-diff form and found no behavioral divergence beyond the intended staging: stats accounting is identical (ensure_in_aube_dir owns the exists() → packages_cached gate the callers did inline), the internal exists() gate shadows no caller precondition (the Directory/Portal forced re-materialize and the GVS eject path both establish non-existence first), and relative sibling symlinks stay invariant under the wrapper rename while Windows junctions are built against final_base_dir. ensure_in_aube_dir also predates this PR and is already exercised by the fetch-time materializer and the existing concurrency test, so this is a consolidation onto proven code rather than new staging logic.
ℹ️ The Windows half of both fixes is never executed by any CI job
Both mechanisms this PR adds are #[cfg(windows)], and no job runs the crate's tests on Windows. aube-linker is not a member of nub's root workspace, so nub's own cargo test never reaches it — and the workflow that does (aube-parity.yml, cargo test --workspace inside vendor/aube) is pinned to ubuntu-latest.
The practical consequence, per new mechanism:
with_transient_retry's retry loop — compiled by the Windows release build, executed by nothing.sharing_violation_is_retriable_only_on_windows— asserts== cfg!(windows), so on the only OS that runs it the assertion reduces to "os 32 is not retriable". The branch the fix exists for is never checked.
Not a defect in the diff, but the fix for a Windows-only bug currently rests entirely on reading the code.
Technical details
# The Windows half of both fixes is never executed by any CI job
## Affected sites
- `.github/workflows/aube-parity.yml` — the `cargo test --workspace` job is `runs-on: ubuntu-latest` ("ubuntu-latest only for now; macOS legs come later"). It is the only job that runs `aube-linker`'s unit tests.
- Root `Cargo.toml` — `aube-linker` is a path dependency, not a workspace member. `cargo test -p aube-linker` from the repo root fails with "package `aube-linker` cannot be tested because it requires dev-dependencies and is not a member of the workspace", so nub's own test legs (Windows included) never build its `#[cfg(test)]` modules.
- `vendor/aube/crates/aube-linker/src/sweep.rs:72-96` — `with_transient_retry`'s loop, `#[cfg(windows)]`, no test coverage on any platform.
- `vendor/aube/crates/aube-linker/src/materialize.rs:1422-1434` — the assertion that carries the fix is `cfg`-conditioned to the platform CI does not run.
## Required outcome
- The `is_transient_fs_error` / `with_transient_retry` behavior on os 5 and os 32, and the retry-then-give-up boundary, are exercised on a real Windows runner (or by a platform-independent test that injects the error rather than depending on `cfg`).
## Suggested approach (optional)
Cheapest option is a Windows leg for the existing job — `runs-on: ${{ matrix.os }}` over `[ubuntu-latest, windows-latest]` — gated off `pull_request` if Windows runner cost is the concern, the same way `aube-conformance-windows` already is in that workflow.
An alternative that costs no runner minutes: make `with_transient_retry` testable without `cfg(windows)` by having it take the transient predicate (or by keeping the loop unconditional and letting a non-Windows `is_transient_fs_error` return `false`). The loop's backoff/attempt-budget behavior would then be assertable everywhere, leaving only the raw-errno mapping platform-specific.
For a one-off confirmation before merge, the repo's `ci-adhoc-test` skill runs a branch-scoped probe on a real `windows-latest` runner with no PR gating.Claude Opus ｜ 𝕏
| /// retry: they already fall through to this copy, so retrying them too would | ||
| /// double the worst-case stall per file for no added recovery. | ||
| fn copy_through_transients(src: &Path, dst: &Path) -> io::Result<u64> { | ||
| crate::sweep::with_transient_retry(|| std::fs::copy(src, dst)) |
There was a problem hiding this comment.
sweep.rs) as "~10s worst case … short enough that a genuinely stuck file fails the install rather than hanging it" — reasoning that holds for one directory removal per install, but not for a per-file call. There is no shared deadline or attempt budget across files, so on a LinkStrategy::Copy install (the cross-volume Windows case the strategy docs call out) every file in every index can independently pay up to ~9.15s of sleep.
Technical details
# Per-file reuse of a ladder tuned for one-shot use
## Affected sites
- `vendor/aube/crates/aube-linker/src/sweep.rs:64-96` — `with_transient_retry`: 10 attempts, 50ms doubling to a 2s cap. Sleep total across attempts 0-8 is 9150ms. Its doc justifies that budget on the assumption of a single call.
- `vendor/aube/crates/aube-linker/src/materialize.rs:79-81` — `copy_through_transients` applies it per file.
- `vendor/aube/crates/aube-linker/src/materialize.rs:865, 941, 956, 963` — the four call sites. On Windows with `LinkStrategy::Copy` (store and project on different volumes) the fourth is every file; the hardlink fallback at 956 is every file on EXDEV.
## Required outcome
- A systemically slow-to-clear source (a real-time AV hook or an indexer sweeping the CAS store while the link pass runs) degrades install wall time by a bounded factor rather than by 9s × files-affected.
- `with_transient_retry`'s doc no longer implies a per-install bound while being called per file.
## Suggested approach (optional)
Either give the per-file path its own shorter ladder (the transient this guards clears in milliseconds when it clears at all — 3-4 attempts topping out near 400ms recovers essentially the same set), or thread a shared `Instant` deadline through the retry so the whole link pass has one budget instead of one per file. Also worth noting: `is_transient_fs_error` matches raw 5 (`ERROR_ACCESS_DENIED`), which is frequently *not* transient, so a genuine ACL problem pays the full ladder before surfacing.
## Open questions for the human
- Is a per-file budget acceptable given `LinkStrategy::Copy` is the cross-volume Windows default, or should the copy path get its own ladder?| if std::fs::read_link(&local_aube_entry).is_ok() { | ||
| let _ = std::fs::remove_dir(&local_aube_entry) | ||
| .or_else(|_| std::fs::remove_file(&local_aube_entry)); | ||
| try_remove_entry(&local_aube_entry); |
There was a problem hiding this comment.
ℹ️ This removal is what the new hard error below depends on, but try_remove_entry neither retries nor reports: it swallows both remove_dir_all and remove_file errors. So on Windows a transient ERROR_SHARING_VIOLATION on this junction — the exact failure class the rest of this PR teaches the linker to ride out — now aborts the install with a synthetic "failed to remove stale shared-store link" message that hides the real errno.
Technical details
# The new hard failure is reached through a non-retrying, error-swallowing removal
## Affected sites
- `vendor/aube/crates/aube-linker/src/link.rs:445` — `try_remove_entry(&local_aube_entry)` on the GVS disk-materialize eject path.
- `vendor/aube/crates/aube-linker/src/sweep.rs:162-165` — `try_remove_entry` is `let _ = remove_dir_all(...); let _ = remove_file(...)`. Deliberately best-effort, which was fine while the caller ignored the outcome; this PR makes the outcome fatal.
- `vendor/aube/crates/aube-linker/src/sweep.rs:98-120` — `remove_dir_all_with_retry` already exists for precisely this class of error and is not used here.
This path is live under nub: `crates/nub-cli/src/pm_engine/mod.rs:2224` populates `diskMaterializePackages` for Vite projects under the machine-global store, which is what routes execution into this branch.
## Required outcome
- A transient os 32 / os 5 on the stale junction is retried before the install is failed.
- When the removal genuinely cannot succeed, the returned `Error::Io` carries the underlying `io::Error` instead of `io::Error::other("...")`, so the report names the real cause.
## Suggested approach (optional)
Replace the best-effort call with a removal whose result is propagated and whose retry goes through `with_transient_retry`, then keep the `symlink_metadata` re-check as the belt-and-suspenders guard it is now. Note `remove_dir_all_with_retry` alone may not cover both shapes here (a junction on Windows vs. a symlink on Unix), which is why `try_remove_entry` tries both calls — a small retrying variant that keeps the last error is probably the cleaner fit.
The sibling site at `link.rs:234-244` has the same shape and predates this PR, so whatever helper lands here should be reusable there.…ect 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
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
There was a problem hiding this comment.
Important
Two of the new retry wrappers need a second look before merge: one spends multi-second backoff inside a sweep loop that throws the result away, and one covers the cooler of two structurally identical parent-directory batches.
Reviewed changes — the single new commit f2b23bd, which extends the Windows transient-retry ladder to the paths 0b816d9 left uncovered and stops a benign junction collision from failing an install.
- Retry the top-level entry removal —
reconcile_top_level_link's Windows removal now goes through the ladder, so anode_modules/<name>held open by a dev server, a--watchtask, or Defender mid-scan no longer aborts a whole reinstall. - Retry the shared entry wipe and the recursive mkdir —
try_remove_entryandmkdirpboth back off now.mkdirpalso moved offxx::file::mkdirpontostd::fs::create_dir_allso the raw errno survives the retry predicate, and it returns a path-carryingError::Ioinstead of the lossyError::Xx. - Retry the hoisted parent-directory batch — the per-file fallback in
hoisted.rscreates every intermediate parent in one pass up front; that pass is now retried. - Accept
ERROR_ALREADY_EXISTSon a junction that already points where we wanted — the newjunction_points_atcompares both sides throughcanonicalize, so the concurrent-writer race behind #576 succeeds while a junction pointing elsewhere still fails. Two Windows-only tests cover both directions. - Reflow six hunks in
patches.rs— rustfmt output, no behavior change.
I traced the os-183 acceptance across every create_dir_link caller and found it sound: junction_points_at requires both paths to canonicalize equal, so a wrongly-targeted junction, a plain directory left by an older layout, and a canonicalize failure all still surface as errors — and no caller consumes 183 as a signal. The mkdirp rewrite likewise checks out against xx::file::mkdirp's actual source (an exists() guard plus the same create_dir_all), apart from the two differences noted inline, and Error::Io's "I/O error at {path}: {err}" is a strict upgrade over Error::Xx's stringified wrap.
Claude Opus ｜ 𝕏
| crate::sweep::with_transient_retry(|| std::fs::create_dir_all(parent)) | ||
| .map_err(|e| Error::Io(parent.clone(), e))?; |
There was a problem hiding this comment.
materialize.rs:633-635 — the isolated linker's per-file fallback, and the hotter of the two — still calls bare std::fs::create_dir_all, so "the remaining transient-fatal paths" is only half covered. This expression is now byte-equivalent to crate::mkdirp, so routing both loops through it closes the gap and drops the duplication in one move.
Technical details
# Fatal `create_dir_all` sites left unretried alongside the one that was fixed
## Affected sites
- `vendor/aube/crates/aube-linker/src/hoisted.rs:820-821` — the site this commit wrapped. `with_transient_retry(|| create_dir_all(parent)).map_err(|e| Error::Io(parent.clone(), e))?` is exactly what the new `mkdirp` (`sweep.rs:177-190`) does.
- `vendor/aube/crates/aube-linker/src/materialize.rs:633-635` — the same batch-every-unique-parent loop in `materialize_into`'s non-clonedir path, still `std::fs::create_dir_all(parent).map_err(|e| Error::Io(parent.clone(), e))?`. This runs for every non-clonedir package, i.e. the dominant path on Windows.
- `vendor/aube/crates/aube-linker/src/materialize.rs:1064`, `1207`, `1247` — three more fatal `create_dir_all` calls (clone parent, trees dir, per-file parent) with no retry.
- `vendor/aube/crates/aube-linker/src/link.rs:537-539` and `1121-1123` — the GVS `Stale` reconciliation still uses a single-attempt, unchecked `remove_dir().or_else(remove_file())`, while every other removal this PR touched was upgraded.
## Required outcome
- Every fatal directory-creation site in the linker rides the same ladder, so a `pending-delete` / `ACCESS_DENIED` slot does not abort an install on one path while being ridden out on another.
- The `hoisted.rs` loop does not carry a hand-inlined copy of `mkdirp`'s body.
## Suggested approach
Replace both loop bodies with `mkdirp(parent)?` (`crate::mkdirp` in `hoisted.rs`, the already-imported `mkdirp` in `materialize.rs`). `parent` is a `&PathBuf` in both, which coerces to the `&Path` parameter. `materialize.rs:1064`/`1207`/`1247` are the same one-line substitution.
## Open questions for the human
- Were `link.rs:537-539` / `1121-1123` left alone deliberately (the os-183 acceptance makes a failed removal there recoverable), or are they simply the next ones in line?| crate::sweep::with_transient_retry(|| std::fs::create_dir_all(parent)) | |
| .map_err(|e| Error::Io(parent.clone(), e))?; | |
| crate::mkdirp(parent)?; |
| with_transient_retry(|| std::fs::create_dir_all(dir)) | ||
| .map_err(|e| Error::Io(dir.to_path_buf(), e)) |
There was a problem hiding this comment.
ℹ️ Dropping the exists() short-circuit costs a failing mkdir plus an is_dir() stat on every warm call where the old path was a single stat, and mkdirp runs per package (link.rs:307, 691, 722, 890, 1071, 1426, 1445, 1468, 1693, 1806). The comment's "dropping the check changes nothing" also isn't quite right: an existing non-directory used to return Ok and now errors — an improvement, but not nothing.
Technical details
# The `exists()` fast path was load-bearing for syscall count
## Affected sites
- `vendor/aube/crates/aube-linker/src/sweep.rs:177-190` — the rewritten `mkdirp`.
- `xx::file::mkdirp` (xx 2.6.1) is `if path.exists() { return Ok(()) }` then `fs::create_dir_all`. `std::fs::create_dir_all` on an existing directory does `mkdir` (fails `AlreadyExists`) then `path.is_dir()` — two syscalls where the old guard was one.
- `vendor/aube/crates/aube-linker/src/materialize.rs:579-597` — the sibling comment documents collapsing "~25 redundant statx calls" per package as worth optimizing, which sets the bar this regresses against.
- Behavior delta: for a path existing as a **file**, `exists()` returned true so the old `mkdirp` returned `Ok`; `create_dir_all` returns `AlreadyExists`. `AlreadyExists` is raw 183 on Windows, so it is not in `is_transient_fs_error`'s `{5, 32}` and does not enter the ladder.
## Required outcome
- The warm path keeps its single-stat cost while the retry and the raw-errno-preserving error conversion stay.
- The comment reflects the actual delta rather than claiming equivalence.
## Suggested approach
Re-add the guard as `is_dir()` rather than `exists()`, which keeps the new (stricter, better) behavior for an existing non-directory while restoring the fast path.| with_transient_retry(|| std::fs::create_dir_all(dir)) | |
| .map_err(|e| Error::Io(dir.to_path_buf(), e)) | |
| if dir.is_dir() { | |
| return Ok(()); | |
| } | |
| with_transient_retry(|| std::fs::create_dir_all(dir)) | |
| .map_err(|e| Error::Io(dir.to_path_buf(), e)) |
| if file_lines.len() < context_index | ||
| || file_lines.len() - context_index < hunk.original_length | ||
| { | ||
| if file_lines.len() < context_index || file_lines.len() - context_index < hunk.original_length { |
There was a problem hiding this comment.
ℹ️ These six hunks are rustfmt reflow with no behavior change and no relation to the linker fix. Nothing gates formatting here — root cargo fmt --check only covers workspace members and vendor/aube is excluded, and aube-parity.yml has no fmt step — so this is conflict surface the next upstream aube sync has to reconcile rather than a gate being satisfied.
There was a problem hiding this comment.
ℹ️ The bounded ladder resolves the sweep-loop stall. One arithmetic discrepancy in its doc, inline.
Reviewed changes — the new commit 691b24a, which splits the retry ladder so a best-effort cleanup pass stops paying the full backoff.
- Split
with_transient_retryinto a thin wrapper overwith_transient_retry_bounded— the attempt count is now a parameter, the Unix passthrough moved into the shared body, and the 10-attempt path keeps its "failure is fatal to the install" framing. - Bound
try_remove_entryto 4 attempts — the best-effort entry wipe that runs once per swept entry no longer multiplies the full ladder across a wholenode_modules/, which was the concern raised on the prior revision.
Everything else in the split checks out: the parameter's #[cfg_attr(not(windows), allow(unused_variables))] is correct since attempts is genuinely unused off Windows, attempts.max(1) keeps the trailing unreachable! sound even for a zero argument, and the Unix path still calls op() exactly once.
Claude Opus ｜ 𝕏
| } | ||
|
|
||
| /// Short ladder for a BEST-EFFORT operation the caller proceeds past either | ||
| /// way: 4 attempts, ~750ms worst case. |
There was a problem hiding this comment.
ℹ️ Four attempts sleeps 350 ms, not ~750 ms: the back-off runs after attempts 0, 1, and 2 (50 + 100 + 200) and attempt 3 returns the error without sleeping. That matters here because the doc justifies the budget against a window that "clears well inside a second" — 350 ms is not comfortably inside that claim, so five attempts is what actually buys the 750 ms being described.
Technical details
# The bounded ladder's documented budget is 2× its actual budget
## Affected sites
- `vendor/aube/crates/aube-linker/src/sweep.rs:75` — "4 attempts, ~750ms worst case".
- `vendor/aube/crates/aube-linker/src/sweep.rs:97-105` — the loop sleeps only when it is *not* about to return, so N attempts produce N-1 sleeps: `50 << attempt.min(6)` for attempt 0, 1, 2 = 350 ms total.
- The same off-by-one is harmless on the full ladder (9 sleeps = 9150 ms, documented as "~10s" at line 66), which is why it reads as correct there.
## Required outcome
- The stated worst case matches the code, and the chosen budget matches the reasoning the doc gives for it.
## Suggested approach
Either bump the call at `sweep.rs:187` to `with_transient_retry_bounded(5, ...)` (50 + 100 + 200 + 400 = 750 ms, matching both the doc and the sub-second rationale) or correct the doc to `4 attempts, ~350ms worst case`.
## Open questions for the human
- Was 750 ms the intended ceiling, or was 4 the intended attempt count?# Conflicts: # vendor/aube/crates/aube-linker/src/link.rs # vendor/aube/crates/aube-linker/src/sweep.rs
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
There was a problem hiding this comment.
Important
The merge resolves the one conflict that main had also just changed in favor of the branch, which shortens a retry ladder main deliberately lengthened two commits ago. Inline.
Reviewed changes — the merge commit c0ace66, which brings main into the staging branch. The branch forked at dc27843e, so the merge pulls in #595 (the overlapping Windows relink fix) and #601 (quarantine::strip_cached_entry) among others; the only substantive delta is how three conflicted regions were resolved.
- Reordered
link_allstep 1a around main's cached-entry strip — the warm branch keeps its ownexists()test sostrip_cached_entrystill runs on a hit, and the cold branch callsensure_in_aube_dir. - Applied the same shape to
link_workspacestep 1a —exists()→ strip →continue, otherwise the staged materialize. - Kept the branch's 4-attempt ladder in
try_remove_entry— main had just moved that function onto the full 10-attempt ladder, so the resolution changes shipped behavior rather than merely reconciling text.
I traced the three merge interactions that could have gone wrong and all three hold. #601's quarantine strip still fires under staging, because materialize_into strips pkg_nm_dir, which is derived from base_dir — the .tmp-* dir — and xattrs survive the rename into place. The os-183 junction acceptance short-circuits ahead of #595's new clear-and-retry wrapper, so a correct junction becomes a no-op while a wrongly-targeted one still falls through and errors. And the staging premise survives: materialize_into's only callers remain the two helpers that stage.
Claude Opus ｜ 𝕏
| // PER LOCKED ENTRY, turning a cleanup pass into a multi-minute stall when | ||
| // a dev server holds several. The transient here is an AV scan window, | ||
| // which clears well inside a second. | ||
| let _ = with_transient_retry_bounded(4, || std::fs::remove_dir_all(path)); |
There was a problem hiding this comment.
try_remove_entry through remove_dir_all_with_retry — so resolving the conflict in favor of the branch narrows the transient window from ~9.15 s to 350 ms for all 14 callers, including the three that abort the install the moment the removal fails (link.rs:235, link.rs:451, link.rs:1026). The 4-attempt bound was asked for on behalf of the six discarded-outcome sweep loops; the other eight callers refill the slot, where a failed wipe is fatal rather than best-effort.
Technical details
# The merge shortens main's `try_remove_entry` ladder at the fatal call sites
## Affected sites
- `vendor/aube/crates/aube-linker/src/sweep.rs:194` — the third of the merge's three conflict resolutions. Base (`cff8bc74`, #595): `remove_dir_all_with_retry(path)`, i.e. `with_transient_retry` = 10 attempts, 50 ms doubling to a 2 s cap = 9150 ms of sleep. Merged: `with_transient_retry_bounded(4, …)` = 3 sleeps = 350 ms.
- Callers that hard-error the install the instant the entry survives: `link.rs:235` and `link.rs:1026` (`"failed to remove stale local dependency materialization"`), `link.rs:451` (`"failed to remove stale shared-store link…"`, added by this PR).
- Callers that refill the slot, so a failed wipe resurfaces as `ERROR_ALREADY_EXISTS` / os 183 from the following `create_dir_link`: `link.rs:547`, `link.rs:901`, `materialize.rs:417`, `hoisted.rs:742`, `hoisted.rs:781`.
- Callers the short ladder was introduced for, where the outcome is discarded: `sweep.rs:275`, `sweep.rs:289`, `sweep.rs:347`, `sweep.rs:350`, `sweep.rs:353`, `link.rs:1252`.
- The doc immediately above (`sweep.rs:74-83`) scopes the short ladder to "a BEST-EFFORT operation the caller proceeds past either way", which describes 6 of the 14 callers.
## Required outcome
- A sweep pass stays bounded per entry (the reason for `691b24a`), while the callers whose failure aborts the install keep at least the tolerance main ships today.
## Suggested approach
Split the two families instead of having one helper serve both: keep `try_remove_entry` on `with_transient_retry` for the refill and hard-error callers, and give the six sweep sites a short-ladder variant (or pass the attempt count at the call site). This is the split the earlier sweep-loop thread proposed, and it satisfies both constraints at once.
## Open questions for the human
- Was shortening #595's ladder intentional, or an artifact of resolving the conflict in favor of the branch?
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single new commit 999d556, pushed while the previous review was in flight. It is a clean revert with no other edits.
- Reverted the os-183 junction tolerance — the
ERROR_ALREADY_EXISTS-is-success branch increate_junction_with_retry, thejunction_points_athelper, and both Windows-only tests are gone;sys.rsis now byte-identical tomain, and the PR is down to six files.
The revert leaves no gap: #576 is already closed by #595, whose create_dir_link wrapper clears the slot and retries once on AlreadyExists, which covers the same collision the tolerance was written for. It also retires the coverage concern raised on sys.rs — that thread is resolved.
Not approving only because the earlier revisions' threads are still open, including the try_remove_entry ladder point from the review a moment ago; none of them are re-raised here.
Claude Opus ｜ 𝕏
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
A link that failed partway left a half-written package under
node_modules/.store, which the next run'sexists()gate counted as already linked — installs then printed "Already up to date" over an unresolvablenode_modules, recoverable only by deleting it.Five
link.rssites wrote straight into the final entry. All now stage through.tmp-<pid>-<id>+ atomic rename, so a failure leaves only a sweepable tmp dir.Retried, each previously fatal: the per-file copy on a Windows sharing violation; the stale top-level entry removal;
mkdirp.is_transient_rename_errornever matched os 32 at all, because Rust decodes it toUncategorized.Prevention only — an already-broken tree still needs one
rm -rf node_modules.Merged with #595, which independently fixed the overlapping Windows failure and closed #566/#576.
Closes #552