-
Notifications
You must be signed in to change notification settings - Fork 708
Add thread-safe status tracking to pipeline steps #12312
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.
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12312Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12312" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds status tracking capabilities to pipeline steps, enabling the pipeline execution engine to monitor the lifecycle state of each step through five distinct statuses: Pending, Running, Succeeded, Failed, and Canceled. This enhances visibility into pipeline execution and supports better diagnostics.
Key Changes:
- Introduced
PipelineStepStatusenum with five lifecycle states - Added automatic status management in
DistributedApplicationPipelinethat updates step status throughout execution - Included comprehensive test coverage with 7 new tests validating all status transitions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/Pipelines/PipelineStepStatus.cs | New enum defining the five possible execution states for pipeline steps |
| src/Aspire.Hosting/Pipelines/PipelineStep.cs | Added Status property with default value of Pending |
| src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs | Implemented automatic status tracking logic throughout step execution lifecycle |
| tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs | Added 7 new tests covering all status transition scenarios |
| catch (OperationCanceledException) | ||
| { | ||
| // Cancellation - mark as canceled and re-throw | ||
| step.Status = PipelineStepStatus.Canceled; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate status assignment for Canceled state. The status is already set to Canceled in the inner catch block (lines 386-390). This outer catch will only execute if the inner try-catch is bypassed, but the inner catch already handles OperationCanceledException and re-throws it. Consider removing this outer catch block to eliminate the redundancy.
| step.Status = PipelineStepStatus.Canceled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 526ca72. Removed the duplicate outer catch block for OperationCanceledException - status is now only set once in the inner catch block.
| catch (Exception ex) | ||
| { | ||
| // Execution failure - mark as failed, cancel all other work, and re-throw | ||
| step.Status = PipelineStepStatus.Failed; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate status assignment for Failed state. The status is already set to Failed in the inner catch block (line 393). This outer catch will receive the re-thrown exception from the inner catch, causing the Failed status to be set twice. The outer assignment on line 416 is redundant.
| step.Status = PipelineStepStatus.Failed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 526ca72. Removed the duplicate outer catch block for Failed status - status is now only set once in the inner catch block.
| using var builder = TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish, publisher: "default", isDeploy: true); | ||
| var pipeline = new DistributedApplicationPipeline(); | ||
|
|
||
| var statusTransitions = new Dictionary<string, List<PipelineStepStatus>>(); |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'statusTransitions' is declared but never used in this test. Consider removing this unused variable declaration.
| var statusTransitions = new Dictionary<string, List<PipelineStepStatus>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 526ca72. Removed the unused statusTransitions variable declaration.
|
|
||
| // Wrap the dependency failure with context about this step | ||
| var wrappedException = new InvalidOperationException(message, ex); | ||
| step.Status = PipelineStepStatus.Failed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to worry about concurrency at all? Is it possible for 2 threads to try to update the status at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all state changes need to be guarded and thread safe. We need to make sure that transitions are valid (you can't flip between arbitrary states).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 526ca72. Made status changes thread-safe by:
- Changed
Statusto a read-only property backed by a private field - Added
TryTransitionStatus()internal method that uses a lock for thread-safe updates - Implemented state transition validation to ensure only valid transitions are allowed (e.g., Pending→Running, Running→Succeeded/Failed/Canceled, but no transitions from terminal states)
- Added 3 new tests to validate the state transition logic
|
@copilot implement the review feedback. Make the state change a method instead of a property and guard against invalid state transitions. |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Implemented in commit 526ca72. Changes made:
All 74 tests pass. |
|
@copilot We should be setting the state to Canceled for OperationCancelledException. We should add a Skipped status if a task was not run. |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Implemented in commit 966cece. Changes made:
All 76 tests pass. |
Summary
This PR implements thread-safe status tracking for pipeline steps, allowing the pipeline execution engine to track whether steps are pending, running, succeeded, failed, canceled, or skipped. This provides better visibility into pipeline execution state and enables improved error reporting and diagnostics.
Changes
New
PipelineStepStatusEnumAdded a new enum with six status values representing the lifecycle of a pipeline step:
Updated
PipelineStepClassModified the
PipelineStepclass to support thread-safe status management:Statusto a read-only property backed by a private field with lock-based synchronizationTryTransitionStatus()method for validated, thread-safe state transitionsPending→Running,Failed(dependency failure), orSkipped(filtered out)Running→Succeeded,Failed, orCanceledSucceeded,Failed,Canceled,Skipped) cannot transition to other statesAutomatic Status Management
Modified the
DistributedApplicationPipelineexecution engine to automatically update step status throughout the execution lifecycle using the thread-safe transition method:Pending→Running: Status is set when step execution beginsRunning→Succeeded: Status is set when step completes successfullyRunning→Failed: Status is set when step throws an exceptionRunning→Canceled: Status is set when step is canceled viaOperationCanceledExceptionPending→Failed: Dependent steps are marked as failed when their dependencies failPending→Skipped: Steps that are filtered out (not in the execution path) are marked as skippedThe implementation ensures that
OperationCanceledExceptionis properly preserved (not wrapped) to enable correct cancellation detection, and eliminates duplicate status assignments in exception handling paths.Testing
Added comprehensive test coverage with 12 new tests covering:
All 76 pipeline tests pass (64 existing + 12 new) with no regressions.
Benefits
Breaking Changes
None. This is a purely additive feature. The status property remains publicly readable, and the internal transition method is only used by the pipeline execution engine. The status property is automatically managed by the pipeline execution engine and requires no changes from consumers.
Fixes #12282
Original prompt
Fixes #12282
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.