Skip to content

PublishingActivityProgressReporter fixes - #8571

Merged
Mitch Denny (mitchdenny) merged 6 commits into
mainfrom
mitchdenny/fix-8570
Apr 7, 2025
Merged

PublishingActivityProgressReporter fixes#8571
Mitch Denny (mitchdenny) merged 6 commits into
mainfrom
mitchdenny/fix-8570

Conversation

@mitchdenny

@mitchdennyMitch Denny (mitchdenny) commented Apr 6, 2025

Copy link
Copy Markdown
Member

Fixes: #8570

This PR fixes a race condition that existed with the PublishingActivityProgressReporter API. The issue was that when someone used the old API to update the status/create the status of an issue it was possible for another piece of code to mutate the state of the publishing activity.

This in turn resulted in internal logic in the AppHostRpcTarget detecting that a publishing activity was completed and yield returning the activity stream without actually transmitting the completed activity.

This resulted in the CLI interpretting this as a failure state (even though the apphost returned a zero exit code).

The fix was to rework the API for the PublishingActivityProgressReporter so that it is similar to what we do with the ResourceNotificationService. Now we have an API where we create a new status object based on the old one.

Here is an example of the new usage:

// Create activityvarpublishingActivity=awaitactivityReporter.CreateActivityAsync("publishing-artifacts",$"Executing publisher {executionContext.PublisherName}",isPrimary:true,stoppingToken).ConfigureAwait(false);// Update activity status.awaitactivityReporter.UpdateActivityStatusAsync(publishingActivity,(status)=>statuswith{IsComplete=true},stoppingToken).ConfigureAwait(false);

Here is a video of Kubernetes publisher working where this issue was first noticed:

aspire-activityreporter-fixes.mp4

CopilotAI review requested due to automatic review settings April 6, 2025 01:47
@github-actionsgithub-actionsBot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 6, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Aspire.Cli/Commands/PublishCommand.cs:178

  • [nitpick] The variable 'lastPublishingActivity' is assigned within the loop but never used elsewhere. Consider removing it if it is redundant, or integrate it in subsequent logic if it was intended to be used.
 (string Id, string StatusText, bool IsComplete, bool IsError)? lastPublishingActivity = null;

Comment threadsrc/Aspire.Hosting/Publishing/PublishingActivityProgressReporter.cs Outdated
Comment threadsrc/Aspire.Cli/Commands/PublishCommand.cs
@mitchdenny
Mitch Denny (mitchdenny) deleted the mitchdenny/fix-8570 branch April 7, 2025 01:41
@mitchdenny

Copy link
Copy Markdown
MemberAuthor

/backport to release/9.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14298999219

@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 7, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-modelIssues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplicationarea-cli

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aspire publish with kubernetes publisher fails the first time

3 participants

@mitchdenny@davidfowl