Skip to content

[rush] Fix issue where package.json script hangs when accessing STDIN on Windows #5579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
octogonz wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[rush] Fix issue where package.json script hangs when accessing STDIN on Windows #5579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07ba090
Fix a longstanding bug where a Node.js child process could hang when …
octogonz Jan 29, 2026
c2c60d0
rush change
octogonz Jan 29, 2026
61a638a
PR feedback
octogonz Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/@microsoft/rush/octogonz-rush-stdin-hang_2026-01-29-03-43.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Fix a longstanding issue where a package.json script could hang on Windows if it accessed STDIN under certain circumstances",
"type": "none"
}
],
"packageName": "@microsoft/rush"
}
2 changes: 1 addition & 1 deletion libraries/rush-lib/src/utilities/Utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ export class Utilities {
}
});

const stdio: child_process.StdioOptions = handleOutput ? ['pipe', 'pipe', 'pipe'] : [0, 1, 2];
Copy link
Contributor

@dmichon-msft dmichon-msft Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ABSOLUTELY not be inheriting stdin, that would imply that anything the user types would get unexpectedly forwarded to some arbitrary set of child processes.

Copy link
Contributor

@dmichon-msft dmichon-msft Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, in general, the else case that inherits stdin is also not a sensible default; the caller should have to expressly specify if they want stdin, otherwise we should default to ignore.

Copy link
Collaborator Author

@octogonz octogonz Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you are saying certainly makes sense whenrush build is running shell scripts and piping their output to the summarizer. The problem underlying this PR is that we have many code paths that run scripts, but no rigorous indication of which scenarios should accept STDIN.

Tracing the callers of _executeLifecycleCommandInternal():

scenario that runs lifecycle script should accept STDIN? ILifecycleCommandOptions.handleOutput
"hook" scripts in rush.json maybe? originally they were for telemetry only, but what about postRushx? !printEventHooksOutputToConsole from experiments.json
"global" Rush custom commands yes false
rushx yes false
ShellOperationRunner for phased builds no; @dmichon-msft I think this is what you are focusing on true
IPCOperationRunner no? this is used to invoke Heft for multi-project watch mode, so I guess interactive commands don't make sense there? true

I'm not even sure handleOutput should be what determines this; look at its docs:

export interface ILifecycleCommandOptions {
  /**
   * If true, suppress the process's output, but if there is a nonzero exit code then print stderr
   */
  handleOutput: boolean;

Maybe it should be renamed to handleOutputAndIgnoreStdin?

Copy link
Collaborator Author

@octogonz octogonz Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion was this:

- const stdio: child_process.StdioOptions = handleOutput ? [0, 'pipe', 'pipe'] : [0, 1, 2];
+ const stdio: child_process.StdioOptions = handleOutput ? ['ignore', 'pipe', 'pipe'] : [0, 1, 2];

It definitely breaks less table rows. The main impact would be Rush global commands, but I suppose that was already broken with pipe?

Copy link
Collaborator Author

@octogonz octogonz Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the branch with this change.

const stdio: child_process.StdioOptions = handleOutput ? ['ignore', 'pipe', 'pipe'] : [0, 1, 2];
if (ipc) {
stdio.push('ipc');
}
Expand Down