Repository navigation
Fix crash when stopping pipeline with stop token - #28300
Open
Jordan Borean (jborean93) wants to merge 2 commits into
Open
Jordan Borean (jborean93) wants to merge 2 commits into
Jordan Borean (jborean93) wants to merge 2 commits into
Conversation
There is a race condition where `PipelineProcess.Stop()` can be called after `Dispose()` has already disposed the token. This causes the `Stop()` call to raise `ObjectDisposedException` crashing the process. An explicitly registered action on `$PSCmdlet.PipelineStopToken.Register()` can also throw an exception which is not handled in the stop call causing a similar crash. The fix is to ignore `AggregateException` which contains any exceptions raised by the user defined delegates on the stop token and exit early if the token has been disposed. We exit early because `Dispose()` has already done the work and there is nothing left to stop.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not reliably exercise cancellation of a disposed token source or the AggregateException handler, leaving the original crash without dependable regression coverage.
2 open issues
What changed in this PR
This PR hardens pipeline cancellation to prevent shell crashes when stopping races with disposal or a registered stop-token callback throws, addressing #28299.
Changes:
- Return safely when the cancellation source has already been disposed.
- Catch callback exceptions so command stopping can continue.
- Add a callback-failure test and adjust test readiness signals.
| File | Description |
|---|---|
test/powershell/Language/Scripting/PipelineStoppedToken.Tests.ps1 |
Adds callback-failure coverage and moves readiness signals into test scripts. |
src/System.Management.Automation/engine/pipeline.cs |
Handles disposed cancellation sources and exceptions from stop-token callbacks. |
Review effort: Balanced
| { | ||
| _pipelineStopTokenSource.Cancel(); | ||
| } | ||
| catch (ObjectDisposedException) |
Collaborator
Author
There was a problem hiding this comment.
Not sure how this would be possible without adding test only hooks that lock the state to a particular set of steps. This would be very complicated and means we start testing an implementation detail. Ultimately this is a race condition that I don't believe we can really test against.
Ilya (iSazonov)
approved these changes
Nov 3, 2026
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.
PR Summary
There is a race condition where
PipelineProcess.Stop()can be called afterDispose()has already disposed the token. This causes theStop()call to raiseObjectDisposedExceptioncrashing the process. An explicitly registered action on$PSCmdlet.PipelineStopToken.Register()can also throw an exception which is not handled in the stop call causing a similar crash.The fix is to ignore
AggregateExceptionwhich contains any exceptions raised by the user defined delegates on the stop token and exit early if the token has been disposed. We exit early becauseDispose()has already done the work and there is nothing left to stop.PR Context
Fixes: #28299
It also fixes a related problem where if the delegate registered by a caller could throw which is raised as an
AggregateException. In C# this wouldn't typically happen but if someone in PowerShell provided any ScriptBlock delegate the cancellation will most likely be triggered somewhere that doesn't have a runspace. For example:The fix is to ignore
AggregateExceptionso that this exception, any anything else a registered delegate a C# cmdlet would register, do not bring down the process.This should be considered for backporting as well. While it is a race condition, the issue from #28299 shows that it can crash the whole process.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header