Harden asynchronous cmdlet pipeline operations#630
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41075fa3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f3852238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5487bb82e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d1d0a482
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b7882019f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0b45e5c0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ccd7e0908
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| catch (Exception exception) | ||
| { | ||
| ClearPipes(); | ||
| DisposePipeOnce(); |
There was a problem hiding this comment.
When a non-async hook writes to a stream and then throws before returning its Task, the write has been queued because the output pipe is active, but this catch immediately clears and disposes the pipe without pumping it. For example, WriteObject("started"); throw ...; now loses "started", whereas the previous pipeline-thread path emitted it synchronously; drain already-queued items before propagating the hook exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84dda05b51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var exception = await Record.ExceptionAsync( | ||
| () => Task.Run(() => command.WriteWarning("too-early"))); |
There was a problem hiding this comment.
Task.Run does not guarantee a different managed thread from the one that constructed command; after this async test yields, the queued delegate may reuse that same thread-pool worker. In that case CanAccessPipelineDirectly accepts the call through _constructionThreadId, so the test either invokes base.WriteWarning on an unattached cmdlet and fails intermittently or stops exercising the intended off-thread branch. Use a dedicated Thread or otherwise assert distinct managed thread IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e66e9184e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| catch (Exception exception) | ||
| { | ||
| TryPublish(new PipelineReply(value: null, exception)); | ||
| throw; |
There was a problem hiding this comment.
When an asynchronous hook catches an exception from ShouldProcess, ShouldContinue, or a credential prompt, this branch has already published the exception to that worker but then rethrows it on the pipeline thread as well. Consequently, RunBlockInAsyncCore cancels and terminates the invocation even if the hook handles the exception and intends to continue, unlike a direct PowerShell interaction whose exception can be caught by its caller; publish the rejection without independently aborting the dispatcher.
Useful? React with 👍 / 👎.
| if (Volatile.Read(ref _currentOutPipe) is null) | ||
| return; | ||
|
|
||
| _ = TryQueue(new PipelineItem(progressRecord, PipelineType.Progress)); |
There was a problem hiding this comment.
When post-await code reuses a mutable ProgressRecord and changes fields such as PercentComplete immediately after WriteProgress returns, the queued item retains the same object reference and PumpItem observes the later values. Rapid progress updates can therefore collapse into duplicate final-state records or report incorrect intermediate state; copy the record before placing it on the asynchronous pipe.
Useful? React with 👍 / 👎.
1 participant
Summary - marshal
ThrowTerminatingErrorcalls from asynchronous work back onto the PowerShell pipeline thread - preserve terminating semantics so code after the error cannot continue on the worker - add an async-safe, request-specificShouldContinuebridge alongside the existingShouldProcessand credential bridges - keepDispose()virtual so derived async cmdlets can release owned resources - extend the canonical lifecycle regression coverage for both approved/denied prompts and terminating errors ## WhyPSPublishModuleis the canonical source for Evotec compiled async cmdlets. FileInspectorX exposed the missing terminating-error and derived-cleanup cases, while DbaClientX carried a localShouldContinueextension. Consolidating these here gives consumer repositories one complete source contract without adding a package dependency. Consumer modules will continue to keep their own namespace-adjusted source copy. ## Compatibility Existing cmdlets and parameters are unchanged. Derived async cmdlets can call the standard protectedThrowTerminatingError(ErrorRecord)method after an await and receive normal PowerShell terminating-error behavior. ## Consumer source contract The canonical base keeps Dispose() virtual and ThrowIfStopped() protected for derived cmdlets. Async hooks start on the pipeline thread and must remain async end to end; source-copy consumers retain their own namespace and any repository-specific helpers. ## Review follow-through Canonical review follow-through adds nullable typed prompt replies, complete ShouldProcess and ShouldContinue overload coverage, public terminating-error compatibility, late-callback rejection, cancellation-on-dispose, immediate pump-failure propagation, and a distinction between PowerShell stop cancellation and operation-local cancellation. The focused canonical contract suite passes 26/26.Final async lifecycle semantics
Synchronous writes and interactions made while an async hook is still on the PowerShell pipeline thread now execute directly after draining earlier worker records. This preserves streaming, collection enumeration, ErrorAction and terminating-error behavior, and FIFO ordering without filling the bounded worker queue. Post-await and background writes remain marshalled to the pipeline thread.
Interactive requests are tied to the lifecycle generation that created them, so stale prompts are rejected. Cancellation and disposal release abandoned reply channels, completed task failures are observed before pumping, and scheduler fallback tasks are disposed. Queue draining is reentrancy-safe, canceled reply waits normalize to PipelineStoppedException, and pre-lifecycle worker writes cannot touch the pipeline API. PSPublishModule remains the canonical two-file source owner; consumer repositories keep namespace-adjusted local copies without a runtime package dependency.