[release/v7.4.15] Close pipe client handles after creating the child ssh process#27139
Conversation
adityapatwardhan
added
the
CL-General
adityapatwardhan
added
the
CL-General
There was a problem hiding this comment.
Pull request overview
Backport to release/v7.4.15 that prevents SSH-based remoting (Invoke-Command -HostName / New-PSSession -HostName) from hanging indefinitely on Windows by ensuring inherited pipe client handles are closed after ssh.exe is created.
Changes:
- Dispose
STARTUPINFOin the Win32CreateProcesspath to close inherited pipe client handles after process creation. - Fix incorrect disposal of
stdOutReaderVar/stdErrReaderVaron exception. - Add CI Pester coverage for “non-existent host” scenarios and minor type/namespace cleanup in remoting transport code.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| ScriptBlock = { New-PSSession -HostName "test-notexist" -UserName "test" -ErrorAction Stop } | ||
| FullyQualifiedErrorId = 'PSSessionOpenFailed' | ||
| }, | ||
| @{ | ||
| Name = 'Verifies no hang for Invoke-Command with non-existing host name' | ||
| ScriptBlock = { Invoke-Command -HostName "test-notexist" -UserName "test" -ScriptBlock { 1 } -ErrorAction Stop } |
There was a problem hiding this comment.
test-notexist is not guaranteed to be non-resolvable (DNS search suffixes and wildcard DNS can resolve it), which can make this CI test flaky or slow. Use a reserved non-existent domain like *.invalid (RFC 2606) and/or pass a small -ConnectingTimeout to keep the test deterministic.
| ScriptBlock = { New-PSSession -HostName "test-notexist" -UserName "test" -ErrorAction Stop } | |
| FullyQualifiedErrorId = 'PSSessionOpenFailed' | |
| }, | |
| @{ | |
| Name = 'Verifies no hang for Invoke-Command with non-existing host name' | |
| ScriptBlock = { Invoke-Command -HostName "test-notexist" -UserName "test" -ScriptBlock { 1 } -ErrorAction Stop } | |
| ScriptBlock = { New-PSSession -HostName "nonexistent.invalid" -UserName "test" -ErrorAction Stop } | |
| FullyQualifiedErrorId = 'PSSessionOpenFailed' | |
| }, | |
| @{ | |
| Name = 'Verifies no hang for Invoke-Command with non-existing host name' | |
| ScriptBlock = { Invoke-Command -HostName "nonexistent.invalid" -UserName "test" -ScriptBlock { 1 } -ErrorAction Stop } |
| Describe "No hangs when host doesn't exist" -Tags "CI" { | ||
| $testCases = @( | ||
| @{ | ||
| Name = 'Verifies no hang for New-PSSession with non-existing host name' | ||
| ScriptBlock = { New-PSSession -HostName "test-notexist" -UserName "test" -ErrorAction Stop } | ||
| FullyQualifiedErrorId = 'PSSessionOpenFailed' | ||
| }, | ||
| @{ | ||
| Name = 'Verifies no hang for Invoke-Command with non-existing host name' | ||
| ScriptBlock = { Invoke-Command -HostName "test-notexist" -UserName "test" -ScriptBlock { 1 } -ErrorAction Stop } | ||
| FullyQualifiedErrorId = 'PSSessionStateBroken' | ||
| } | ||
| ) | ||
|
|
||
| It "<Name>" -TestCases $testCases { | ||
| param ($ScriptBlock, $FullyQualifiedErrorId) | ||
|
|
||
| $ScriptBlock | Should -Throw -ErrorId $FullyQualifiedErrorId -ExceptionType 'System.Management.Automation.Remoting.PSRemotingTransportException' | ||
| } |
There was a problem hiding this comment.
This new CI Describe block doesn’t skip when ssh isn’t available, while other SSH remoting tests do (e.g. SSHRemotingAPI.Tests.ps1 uses a $skipTest guard). To avoid CI failures on images/environments without OpenSSH, add a similar skip condition (or explicitly assert the prerequisite and skip with a clear reason).
| It "<Name>" -TestCases $testCases { | ||
| param ($ScriptBlock, $FullyQualifiedErrorId) | ||
|
|
||
| $ScriptBlock | Should -Throw -ErrorId $FullyQualifiedErrorId -ExceptionType 'System.Management.Automation.Remoting.PSRemotingTransportException' |
There was a problem hiding this comment.
These tests aim to ensure the cmdlets don’t hang, but the current assertion can still hang the entire Pester run if the bug regresses (the remoting call never returns, so Should -Throw never completes). To keep CI reliable, run the remoting call in a background job/process and enforce a hard timeout, failing the test if the timeout is exceeded (and cleaning up the job).
adityapatwardhan
merged commit 5e71058
into
PowerShell:release/v7.4.15
Backport of #26491 to release/v7.4.15
Triggered by @adityapatwardhan on behalf of @daxian-dbw
Original CL Label: CL-General
/cc @PowerShell/powershell-maintainers
Impact
REQUIRED: Choose either Tooling Impact or Customer Impact (or both). At least one checkbox must be selected.
Tooling Impact
Customer Impact
Fixes issues #25203 and #26228 where
Invoke-Command -HostNameandNew-PSSession -HostNamehang indefinitely on Windows when connecting to a non-existent SSH host. On Windows, pipe client handles were not closed afterssh.exewas created, causing the process to remain open even after ssh.exe exited.Regression
REQUIRED: Check exactly one box.
This is not a regression.
Testing
New Pester tests added in
SSHRemotingCmdlets.Tests.ps1that verifyNew-PSSessionandInvoke-Commandwith non-existing hosts return errors instead of hanging indefinitely.Risk
REQUIRED: Check exactly one box.
The fix is surgical - it adds
lpStartupInfo.Dispose()in a finally block after process creation to close inherited pipe client handles. This prevents a hang but doesn't change the core logic. Also includes a bug fix wherestdOutReaderVarandstdErrReaderVarwere incorrectly disposingstdInWriterVar.Merge Conflicts
Conflict in
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs: The release branch uses the 2-paramcontext.CommandDiscovery.LookupCommandInfo(sshCommand, CommandOrigin.Internal)API while the backport uses the 5-param static overload. Resolved by keeping the release branch's API form and adjusting the variable reference fromappInfo.PathtocmdInfo.Path. The key bug fix (lpStartupInfo.Dispose()in the finally block) and other changes applied cleanly.