feat(cli): make download timeout configurable and more forgiving - #2386
Merged
Conversation
Node.js runtime and package-manager tarball downloads shared the HTTP client's hardcoded 2-minute per-request timeout with small metadata fetches. On slow or flaky connections that budget aborts an otherwise healthy transfer, and there was no way to override it. Give large file downloads their own per-request timeout: a 10-minute default, overridable via the VP_DOWNLOAD_TIMEOUT_SECS environment variable. The shared client's 2-minute default stays as-is so a stuck metadata fetch still fails fast. Closes voidzero-dev#2370
✅ Deploy Preview for viteplus-preview canceled.
|
fengmk2
reviewed
Aug 9, 2026
Member
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc82508b2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An extreme but parseable value (e.g. 18446744073709551615) was accepted and the resulting Duration overflowed reqwest's request-deadline computation (Instant + Duration panics; the release profile aborts), so a malformed timeout crashed every download instead of producing the documented warn-and-fallback. Reject values above 86400 (24 hours) with the same warn-and-fallback as other invalid values, and cover the boundary and overflow cases in the unit test. Refs voidzero-dev#2370
wan9chi
pushed a commit
that referenced
this pull request
Aug 10, 2026
… Node version (#2390) Since 2026-08-08 the Windows PTY snapshot leg fails intermittently on `dev_engines_runtime_pnpm11`: the `vp dlx -s print-current-version` step times out after 60s with empty output (13+ runs across branches, main included, e.g. [run 31302523708](https://github.com/voidzero-dev/vite-plus/actions/runs/31302523708)). The fixture pinned devEngines node `22.22.2`, the only pin in the suite that the CI runtime seed does not carry, so the step had to download Node.js from nodejs.org inside its 60s budget. Connections from the Windows runner to nodejs.org stall intermittently, and the shared HTTP client's 2-minute request timeout (see #2386) outlives the step budget, so a stalled attempt can neither fail nor retry in time. The sibling `dev_engines_runtime_pnpm10` pins the seeded default `22.18.0` and passed in ~2s in the same failed runs, which clears the npm registry path and isolates the stall to nodejs.org. Pin the pnpm11 fixture to `22.18.0` as well. The `22.22.2` pin carried no assertion value: #1289 picked it as the then-latest 22.x above vp's minimum, and the snapshot redacts the printed version to `<version>`. The case still verifies that vp honors `devEngines.runtime` under pnpm 11. Verified with `cargo test -p vp_cli_snapshots --test cli_snapshots -- dev_engines_runtime_pnpm11` (passes in 6s, no snapshot drift).
Member
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
fengmk2
added
the
preview-build
Contributor
Registry bridge build (
|
| Package | Version |
|---|---|
vite-plus |
0.0.0-commit.4c9367f409111d88ac9338427886270029bd6b8e |
@voidzero-dev/vite-plus-core |
0.0.0-commit.4c9367f409111d88ac9338427886270029bd6b8e |
Install the Vite+ CLI built from this commit, then migrate a project:
# macOS / Linux
curl -fsSL https://vite.plus | VP_PR_VERSION=2386 bash# Windows (PowerShell)
$env:VP_PR_VERSION="2386"; irm https://vite.plus/ps1 | iexAfter installing, upgrade the current project's vite-plus to this test build with:
vp migrateOr point your package manager at the bridge registry https://registry-bridge.viteplus.dev/:
| Package manager | Registry config |
|---|---|
| npm / pnpm / Bun | .npmrc: registry=https://registry-bridge.viteplus.dev/ |
| Yarn (v2+) | .yarnrc.yml: npmRegistryServer: "https://registry-bridge.viteplus.dev/" |
Then pin the build (vite aliases to vite-plus-core; pnpm can use a catalog, npm an overrides entry):
{
"devDependencies": {
"vite-plus": "0.0.0-commit.4c9367f409111d88ac9338427886270029bd6b8e",
"vite": "npm:@voidzero-dev/vite-plus-core@0.0.0-commit.4c9367f409111d88ac9338427886270029bd6b8e"
}
}
fengmk2
reviewed
Aug 10, 2026
Contributor
Author
|
Good point - renaming to VP_DOWNLOAD_TIMEOUT and keeping plain ints as seconds. Best Regards, Tarik |
Keep the unit out of the name so the value can later accept duration formats like 10m; plain integers remain seconds. Bounds (1..=86400), warning text, and warn-fallback behavior are unchanged.
fengmk2
approved these changes
Aug 11, 2026
fengmk2
added a commit
that referenced
this pull request
Aug 11, 2026
Two snapshot cases intermittently fail the Windows leg with "timed out after 60s" and empty partial output (run 31453833634, attempts 2 and 4; same stall class as #2390): - migration_eslint: the fixture has no Node version pin, so the global vp resolves "latest LTS" through a nodejs.org/dist/index.json fetch before it spawns node and prints anything. Each case runs in a fresh VP_HOME and the CI runtime seed carries no index cache, so every such case fetches the index, and a stalled connection spends the whole 60s step budget with no output. - command_cache_bun: the case provisions bun@1.3.11 into its fresh VP_HOME, and a stalled tarball download holds the request for vp's full download timeout (10 minutes since #2386), so the retry never gets a chance inside the step budget. Two fixes: - Warm the version index cache in the snapshot jobs' prewarm step with `vp env list-remote`, so `node/index_cache.json` lands in the seeded js_runtime dir. nodejs.org serves the index with a one-hour max-age, which covers the whole suite, and vp falls back to an existing cache when a later fetch fails. - Set VP_DOWNLOAD_TIMEOUT=30 (the #2386 knob) in every case's env in the runner, so a stalled runtime or package-manager download fails fast enough for vp's retry to finish inside the 60s step budget. Healthy CI downloads take about a second. Verified with `cargo check -p vp_cli_snapshots --tests` plus local runs of command_cache_bun and dev_engines_runtime_pnpm10 (both pass; the installed vp 0.2.8 ignores the unknown env var, while CI builds vp from HEAD where the cap is active). migration_eslint needs the JS dist that a fresh worktree lacks, so CI covers that case. Claude-Session: https://claude.ai/code/session_015ALfSFSa1Q2BN198bkDbpr
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
2 participants
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 & why
Closes #2370.
Node.js runtime tarballs (~25–50 MB) and package-manager tarballs are downloaded through the process-wide shared HTTP client, whose per-request total timeout is a hardcoded 2 minutes (
crates/vp_shared/src/http.rs). reqwest's timeout covers the entire body stream, so on connections slower than ~250 KB/s a healthy Node.js download is aborted mid-transfer, every retry starts from scratch, andvp env install/vp migrateeffectively cannot complete. There was also no way to override the timeout.This change gives large file downloads their own per-request timeout instead of raising the shared default:
vp_shared::download_timeout()— 10-minute default, overridable with theVP_DOWNLOAD_TIMEOUT_SECSenvironment variable (positive integer seconds; a set-but-invalid value warns and falls back to the default).download_fileimplementations:crates/vp_js_runtime/src/download.rs(Node.js runtimes — alldownload_runtime*callers funnel through it) andcrates/vp_pm_cli/src/request.rs(npm/yarn/pnpm/bun tarballs — all go throughHttpClient::download_file).docs/guide/installer-env-vars.mdalongside the other runtime variables.Verification
main: both download paths issueclient.get(url).send()against the shared client with the hardcoded 2-minute total timeout; a 30–50 MB archive needs a sustained ≥ ~300 KB/s to finish inside that window.cargo check -p vp_shared -p vp_js_runtime -p vp_pm_cli --all-targets --locked— clean.cargo clippy -p vp_shared -p vp_js_runtime -p vp_pm_cli --all-targets --all-features -- --deny warnings(same-Aset asjust lint) — clean.cargo fmt --all— applied, no drift.cargo test -p vp_shared -p vp_js_runtime -p vp_pm_cli— all suites pass (962 passed, 0 failed; new unit tests cover the env parsing: default, valid override, whitespace trimming, and invalid/blank/zero/negative fallback).httpmock-based download tests invp_pm_clistill pass, exercising the patched request path end to end.Notes / limitations
vp upgradeand the standalone installer download thevptarball itself viaget_bytes, which still uses the shared 2-minute default; extending the knob there would touch a different API and is left for a follow-up if desired.AI disclosure
This PR was prepared with AI assistance (code authored and tested by an AI agent, reviewed by a second independent AI reviewer pass whose findings were addressed or noted above).