Skip to content

fix(js-runtime): stop deleting a concurrently-installed runtime#2248

Open
shulaoda wants to merge 1 commit into
mainfrom
07-26-fix_js-runtime_stop_deleting_a_concurrently-installed_runtime
Open

fix(js-runtime): stop deleting a concurrently-installed runtime#2248
shulaoda wants to merge 1 commit into
mainfrom
07-26-fix_js-runtime_stop_deleting_a_concurrently-installed_runtime

Conversation

@shulaoda

Copy link
Copy Markdown
Member

Problem

Installing a managed runtime (download_runtime_with_provider in runtime.rs) runs four steps in order:

step action under lock
1 if the binary (install_dir/.../node) exists, return the cached runtime no
2 if install_dir exists, treat it as an incomplete install and remove_dir_all it no
3 download and extract into a temp dir no
4 move_to_cache: take the per-version lock, re-check, atomic rename into install_dir yes

Only the rename in step 4 is protected by the lock. Step 2 runs without it, and it decides "incomplete" from the directory's existence, not from whether the binary is present.

The race, with two processes installing the same uncached version:

time process A process B
1 step 1: binary absent, continue
2 runs steps 3 and 4, rename completes, so install_dir and the binary now exist (B's complete install)
3 step 2: sees install_dir that B just created, treats it as incomplete, remove_dir_all deletes B's install

Between A's check in step 1 and A's cleanup in step 2, B finished. Because A's cleanup only tests the directory, it wipes B's complete install.

Impact: the cache self-heals because A reinstalls, but B already returned a JsRuntime pointing at install_dir/.../node. During A's remove-then-reinstall window the binary is gone, so a concurrent exec fails with ENOENT.

Root cause: the cleanup ran outside the lock and keyed on the directory instead of the binary, so a complete install was misread as incomplete and deleted.

Fix

move_to_cache takes a new binary_path argument. The completeness check and the cleanup move into it, under the same lock as the rename, and key off the binary:

// Under the lock, decide by the binary (not just the directory) and clean up
// here, so it can't delete a peer's install mid-race:
//   - binary present     -> a peer's complete install, keep it and skip
//   - dir without binary -> a stale/partial install, remove then move
if fs::try_exists(binary_path.as_path()).await.unwrap_or(false) {
    return Ok(());
}
if fs::try_exists(target.as_path()).await.unwrap_or(false) {
    fs::remove_dir_all(target.as_path()).await?;
}
fs::rename(source.as_path(), target.as_path()).await?;

The caller's unlocked cleanup in step 2 is deleted.

Why the race is closed: the completeness check, the removal, and the rename now all happen under one lock and all look at the binary. Replaying the timeline, A reaches move_to_cache, takes the lock after B, sees the binary, and skips, so it discards its own temp download instead of deleting B's install. Recovery of a stale partial install still works, since a directory with no binary is removed under the lock and replaced.

Tests

Three deterministic move_to_cache tests (filesystem only, no network) cover the three target states:

test asserts
move_to_cache_moves_into_absent_target no target, so the source is moved in
move_to_cache_replaces_stale_incomplete_target directory but no binary, so it is removed and replaced
move_to_cache_keeps_a_complete_target_intact directory and binary present, so it is kept (the regression guard)

The cross-process race has a sub-millisecond window and cannot be reproduced deterministically in a unit test (the existing test_concurrent_downloads is #[ignore]d). These tests instead pin the invariant that makes the race safe: move_to_cache never clobbers a complete install and always replaces a partial one. The actual deleter, the unlocked caller-side cleanup, is removed outright.

Verification

cargo test  -p vite_js_runtime --lib download::tests   # 4 passed
cargo clippy -p vite_js_runtime --all-targets          # clean
cargo fmt --all --check                                # clean

@netlify

netlify Bot commented Jul 25, 2026

Copy link
Copy Markdown

Deploy Preview for viteplus-preview canceled.

Name Link
🔨 Latest commit a60c643
🔍 Latest deploy log https://app.netlify.com/projects/viteplus-preview/deploys/6a64fd7878910d000854e7b7

@github-actions

Copy link
Copy Markdown
Contributor

Native binary sizes (a60c643)

Final release artifacts built by the canonical build-upstream and build-windows-cli actions.

Artifact Format Base PR Change
vp (Linux x64) Binary 10.31 MiB 10.31 MiB 0 B (0.00%)
vp (Linux x64) gzip -9 4.42 MiB 4.42 MiB -348 B (-0.01%)
NAPI (Linux x64) Binary 33.07 MiB 33.07 MiB 0 B (0.00%)
NAPI (Linux x64) gzip -9 12.72 MiB 12.72 MiB 0 B (0.00%)
vp (macOS ARM64) Binary 7.64 MiB 7.64 MiB 0 B (0.00%)
vp (macOS ARM64) gzip -9 3.84 MiB 3.84 MiB +175 B (+0.00%)
NAPI (macOS ARM64) Binary 40.50 MiB 40.50 MiB 0 B (0.00%)
NAPI (macOS ARM64) gzip -9 16.97 MiB 16.97 MiB +5 B (+0.00%)
vp (Windows x64) Binary 8.35 MiB 8.35 MiB +1.00 KiB (+0.01%)
vp (Windows x64) gzip -9 3.64 MiB 3.64 MiB -143 B (-0.00%)
NAPI (Windows x64) Binary 27.51 MiB 27.51 MiB 0 B (0.00%)
NAPI (Windows x64) gzip -9 10.70 MiB 10.70 MiB +2 B (+0.00%)
Trampoline (Windows x64) Binary 203.00 KiB 203.00 KiB 0 B (0.00%)
Trampoline (Windows x64) gzip -9 97.91 KiB 97.91 KiB -1 B (-0.00%)
Installer (Windows x64) Binary 4.44 MiB 4.44 MiB +512 B (+0.01%)
Installer (Windows x64) gzip -9 2.08 MiB 2.08 MiB -108 B (-0.00%)

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.

1 participant