Repository navigation
fix(server): stopping a Node.js install waits until its check has really exited - #732
Merged
Merged
Conversation
…lly exited Stopping an install while it ran the downloaded `node --version` check returned as soon as the stop signal was sent: execFile answers an abort right away, before the process exits. The install then went on to delete the folder that process runs from (on Windows that fails while it runs), and the test for this, which checks the process is gone, failed now and then on CI. runOnce now settles only once the program has exited, and a program that ignores the stop is killed outright after two seconds so a stop can't hang. The test's fake node now ignores the polite stop, so the old code fails it every time instead of rarely.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This branch was previously deployed
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.
ManagedNode.test.ts› "an install stopped while Node is being checked leaves no process and no files" failed now and then on CI (seen in #731's merge queue run):process.kill(pid, 0)still found the process right after the install was stopped.Why.
runOncesettled onexecFile's callback. On an abort, Node sends the kill signal and raises theAbortErrorright away, before the process exits. A standalone probe on Node 22 hit this 20 out of 20 times. So a stopped install moved on to deleting the folder the check runs from while the process was still there; on Windows that delete fails while it runs. Usually the process was reaped a moment later, which is why the test mostly passed.Fix.
runOncenow settles only after the child'sexit(or right away when it never started and has no pid). A check that ignores the stop is killed with SIGKILL afterSTOP_GRACE_MS(2 s), so stopping can't hang. The test's fakenodenow ignores SIGTERM, so the old code fails it every time (checked) and the fix passes it.Checks:
vp fmt,vp lint,vp run typecheck;ManagedNodetests three runs in a row.