fix: retry sfw vp install when sfw misreports vp as not found on Windows - #120
Merged
Conversation
sfw resolves the wrapped command on Windows via powershell.exe Get-Command with a hard 10s child-process timeout (sfw-free v1.15.0, resolveWindowsCommand). A cold PowerShell start can exceed that, and sfw reports the killed lookup as "Command 'vp' not found in PATH" even though vp is installed, failing the job ~10.5s after spawn. When an sfw-wrapped install fails with that exact signature, warm the PowerShell command cache with an uncapped Get-Command lookup, then retry the install once. Healthy runs see no overhead; a genuinely missing vp still fails on the retry.
Member
Author
|
@codex review |
A single warmed retry can still lose the race on a heavily loaded runner. Keep retrying while the exact flake signature persists, up to 3 retries, warming the PowerShell command cache before each one. Any other failure still stops the loop immediately.
This reverts commit ac0a3ec.
|
Codex Review: Didn't find any major issues. Swish! 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". |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR mitigates a recurring Windows CI flake where sfw vp install incorrectly fails with Command 'vp' not found in PATH due to a 10s PowerShell resolution timeout inside sfw-free, by adding a one-time retry path that warms PowerShell’s command cache on Windows.
Changes:
- Add flake signature detection and a best-effort Windows PowerShell warm-up helper in
runViteInstall. - Retry
sfw vp installonce when the specific “vp not found” signature is observed. - Add unit tests covering the retry behavior and edge cases; update the bundled
dist/index.mjs.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/run-install.ts | Adds sfw flake detection, PowerShell warm-up, and single retry logic in runViteInstall. |
| src/run-install.test.ts | Adds tests validating retry/warm-up behavior and non-matching failure handling. |
| dist/index.mjs | Updates the compiled action bundle to include the new retry logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+77
| warning( | ||
| "sfw reported vp as not found even though it is on PATH. This is a known sfw flake on Windows: a cold PowerShell start exceeds sfw's 10s command-resolution timeout and the timeout is misreported as not-found. Warming the PowerShell command cache and retrying once.", | ||
| ); |
Comment on lines
+27
to
+29
| await getExecOutput("powershell.exe", ["-NoProfile", "-Command", "Get-Command vp"], { | ||
| ignoreReturnCode: true, | ||
| }); |
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
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.
Fixes the recurring Windows flake where
sfw vp installfails withCommand 'vp' not found in PATH~10.5s after spawn while vp is installed and on PATH (seen in this repo's CI and downstream, e.g. node-modules/urllib#859).Root cause (from the bundled JS inside the sfw-free v1.15.0 binary): on Windows, sfw's
resolveWindowsCommandresolves the wrapped command by runningpowershell.exe -NoProfile -Command "Get-Command vp ..."throughchild_process.execwith a hardtimeout: 10000. When a cold PowerShell start plusGet-Commandmodule discovery exceeds 10s, exec kills the lookup, sfw swallows the error and throws the misleading not-found message. A genuine not-found returns in ~1s, so the ~10.5s timing identifies the timeout path.Change in
runViteInstall: when an sfw-wrapped install fails with that exact signature, log a warning, warm the PowerShell command cache with an uncappedGet-Command vp(Windows only), then retry the install once. The retry runs against warm caches and fits sfw's 10s window. Healthy runs see no extra work, non-matching failures still fail immediately, and a genuinely missing vp fails on the retry.Tests cover the retry, the warm-up call shape, single-retry limit, non-matching failures, sfw-disabled runs, and a throwing warm-up. Worth filing upstream at SocketDev/sfw-free so the timeout itself gets fixed; this retry keeps CI green meanwhile.