feat(cli): make download timeout configurable and more forgiving - #2386
feat(cli): make download timeout configurable and more forgiving#2386tarikermis wants to merge 5 commits into
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 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| ### `VP_DOWNLOAD_TIMEOUT_SECS` | ||
|
|
||
| - **Purpose**: Per-request timeout, in seconds, for large downloads such as Node.js runtimes and package-manager tarballs | ||
| - **Default**: `600` (10 minutes) |
There was a problem hiding this comment.
Where does the default ten-minute reference value come from? Could you refer to what the default values for this parameter are in other Node.js Version Managers?
|
@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
… 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).
|
@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
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"
}
}
| - **Default**: None (verification enabled) | ||
| - **Details**: [Node.js Signature Verification](/guide/env#node-js-signature-verification) | ||
|
|
||
| ### `VP_DOWNLOAD_TIMEOUT_SECS` |
There was a problem hiding this comment.
I’d prefer VP_DOWNLOAD_TIMEOUT over VP_DOWNLOAD_TIMEOUT_SECS, so the value can evolve to support duration formats like 10m without baking the unit into the variable name. Plain integer values could default to seconds.
|
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.
2 participants
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).