FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix `Invoke-Command` missing error on session termination. by PaulHigin · Pull Request #11586 · PowerShell/PowerShell · GitHub

Fix Invoke-Command missing error on session termination. - #11586

Merged
Travis Plunk (TravisEz13) merged 4 commits into
PowerShell:masterfrom
PaulHigin:fix-missing-error-invokecommand
Feb 5, 2020
Merged

Fix Invoke-Command missing error on session termination.#11586
Travis Plunk (TravisEz13) merged 4 commits into
PowerShell:masterfrom
PaulHigin:fix-missing-error-invokecommand

Conversation

Paul Higinbotham (PaulHigin) commented Jan 15, 2020
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

This change fixes a problem in Invoke-Command against a remote session and that session is abruptly terminated, but no error is reported.

PR Context

In certain conditions, an abrupt session termination results in an Invoke-Command pipeline state going to 'Stopped' with an exception. Currently this is ignored in job processing because a user initiated stop is not an error. But a stopped state due to an error that is not 'PipelineStoppedException' (such as a remote transport exception) should not be ignored, but instead be treated as an error so that Invoke-Command will report it.

The fix is to update job error processing to correctly handle this error state.

Repro steps

$session = New-PSSession -cn vm1
Invoke-Command -Session $session -ScriptBlock { 1..1000 | % { sleep 1; "Output $_" } }

# Turn off virtual machine vm1

# Result
# No error is reported by Invoke-Command

# Expected
OpenError: [vm1] Processing data from remote server vm1 failed with the following error message: The request for the Windows Remote Shell with ShellId 8E31C82A-7B31-41F0-B882-E60EF7634D33 failed because the shell was not found on the server. Possible causes are: the specified ShellId is incorrect or the shell no longer exists on the server. Provide the correct ShellId or create a new shell and retry the operation. For more information, see the about_Remote_Troubleshooting Help topic.

PR Checklist

Paul Higinbotham (PaulHigin) added WG-Remoting PSRP issues with any transport layer Breaking-Change breaking change that may affect users Issue-Bug Issue has been identified as a bug in the product Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 15, 2020

Copy link
Copy Markdown
Contributor Author

This is a possible breaking change. Some job ending states that used to be 'Stopped' with Exception, will now be 'Failed' with Exception. But I feel this is the correct behavior. The only time a 'Stopped' state is not an error is if there is no associated exception, or the exception is 'PipelineStopped'.

Copy link
Copy Markdown
Contributor Author

PoshChan-Bot (@PoshChan) Please retry windows

Copy link
Copy Markdown
Collaborator

Paul Higinbotham (@PaulHigin), successfully started retry of PowerShell-CI-Windows

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

are there tests that can be added for this?

Copy link
Copy Markdown
Contributor Author

James Truher (@JamesWTruher) No, the repro is difficult to automate and still get the right error path. We don't need another fragile test.

Steve Lee (SteveL-MSFT) commented Jan 15, 2020
edited
Loading

Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this and agree that the scenario where the pipeline is stopped with an error that is not a PipelineStoppedException it is a failure case, so this change is accepted.

Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 15, 2020

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@TravisEz13) This change created a subtle regression. Please hold off merging until I have pushed the fix.

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) , James Truher (@JamesWTruher) Please re-review the changes. I have added a fix to the regression and also fixed the Stop-Job tests so that they will catch bad final state.

The regression was due to the pipeline returning a RemoteException exception on a pipeline stop instead of a PipelineStoppedException exception I was previously checking. A pipeline stopped RemoteException will contain an ErrorRecord with FQEID of 'PipelineStopped'.

The Stop-Job tests were not catching the incorrect final state because they ran before the job was actually running. Fix is to wait until job is running and returning data to the client.

Dongbo Wang (daxian-dbw) added this to the GA-consider milestone Jan 31, 2020
ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jan 31, 2020

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) Please re-review.

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) ping...

Travis Plunk (TravisEz13) added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Feb 5, 2020
Travis Plunk (TravisEz13) changed the title Fix Invoke-Command missing error on session termination. Fix Invoke-Command missing error on session termination. Feb 5, 2020
Travis Plunk (TravisEz13) merged commit a5a97a5 into PowerShell:master Feb 5, 2020
Travis Plunk (TravisEz13) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 5, 2020

Copy link
Copy Markdown

🎉v7.0.0-rc.3 has been released which incorporates this pull request.:tada:

Handy links:

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

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Bug Issue has been identified as a bug in the product WG-Remoting PSRP issues with any transport layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL