-
Notifications
You must be signed in to change notification settings - Fork 763
Make IReportingStep and IReportingTask completion idempotent #12602
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
Conversation
Co-authored-by: davidfowl <[email protected]>
|
@copilot transitioning from terminal state to a different terminal state is fine. Just noop. |
Co-authored-by: davidfowl <[email protected]>
Updated in c9582ef. All terminal state transitions are now a noop - the implementation simply returns early if the step/task is already in any terminal state, keeping the original state and completion text. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12602Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12602" |
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 modifies the PipelineActivityReporter to make task and step completion operations idempotent. Previously, attempting to complete an already-completed task or step would throw an InvalidOperationException. Now, subsequent completion attempts on terminal states are no-ops.
Key Changes
- Changed exception-throwing behavior to early returns when tasks/steps are already in terminal states (Completed, CompletedWithWarning, CompletedWithError)
- Updated test suite to verify idempotent behavior instead of testing for exceptions
- Added comprehensive test coverage for all terminal state transitions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/Pipelines/PipelineActivityReporter.cs | Changed CompleteStepAsync and CompleteTaskAsync to return early when already in terminal state instead of throwing exceptions |
| tests/Aspire.Hosting.Tests/Publishing/PipelineActivityReporterTests.cs | Replaced exception-testing tests with idempotency tests and added extensive coverage for terminal state transitions |
| await task.CompleteAsync(null, cancellationToken: CancellationToken.None); | ||
|
|
||
| // Assert - No new activity should be emitted (noop) | ||
| Assert.False(reporter.ActivityItemUpdated.Reader.TryRead(out _)); |
Copilot
AI
Nov 1, 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.
[nitpick] The pattern while (reporter.ActivityItemUpdated.Reader.TryRead(out _)) { } followed by Assert.False(reporter.ActivityItemUpdated.Reader.TryRead(out _)) is repeated throughout the new tests. Consider extracting this into a helper method like AssertNoActivitiesEmitted(reporter) to reduce duplication and improve maintainability.
| var exception = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| () => task.CompleteAsync(null, cancellationToken: CancellationToken.None)); | ||
| // Clear activities | ||
| while (reporter.ActivityItemUpdated.Reader.TryRead(out _)) { } |
Copilot
AI
Nov 1, 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.
[nitpick] The activity clearing pattern while (reporter.ActivityItemUpdated.Reader.TryRead(out _)) { } is repeated 10+ times across the new tests. Consider creating a helper method like ClearActivities(reporter) to improve code maintainability and readability.
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.
@copilot apply changes based on this feedback
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.
Refactored in dea6fa8. Added ClearActivities() and AssertNoActivitiesEmitted() helper methods and replaced all 13 instances of the duplicated patterns.
…ities Co-authored-by: davidfowl <[email protected]>
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19015985829 |
Make IReportingStep and IReportingTask completion idempotent
Issue Analysis
The error message
Cannot complete step '333e03eb-984a-4bc1-8163-627f1206228c' with state 'CompletedWithError'. Only 'InProgress' steps can be completed.indicates that a step cannot be completed when it's already in a non-InProgress state. The issue required making state transitions idempotent.Requirements (from agent instructions)
Implementation Summary
Core Changes (Updated per @davidfowl feedback)
✅ Modified
PipelineActivityReporter.CompleteStepAsyncto be fully idempotent:✅ Modified
PipelineActivityReporter.CompleteTaskAsyncto be fully idempotent:Test Updates (Updated per @davidfowl feedback)
ClearActivities()andAssertNoActivitiesEmitted()to reduce code duplicationValidation Results
Behavior Changes
Before: Attempting to complete a step/task that was already completed would throw an exception
After (per @davidfowl feedback):
Fixes #12601
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.