Skip to content

Only fire BeforeResourceStartedEvent when actually starting a resource#13519

Merged
danegsta merged 5 commits intomainfrom
danegsta/onlyBeforeStartOnStart
Dec 12, 2025
Merged

Only fire BeforeResourceStartedEvent when actually starting a resource#13519
danegsta merged 5 commits intomainfrom
danegsta/onlyBeforeStartOnStart

Conversation

@danegsta
Copy link
Member

@danegsta danegsta commented Dec 11, 2025 •
edited
Loading

Description

We currently fire BeforeResourceStartedEvent when creating resources even if a resource is set to explicit start. This means the event can't be used to reliably do work only before a resource is actually starting without doing ref counting. This change skips firing the initial event if the resource is marked for explicit startup.

Fixes #13068
Fixes #13070

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2025 •
edited
Loading

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13519

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13519"

@danegsta
Copy link
Member Author

danegsta commented Dec 12, 2025

@copilot add unit tests to DistributedApplicationTests.cs to verify that these events are firing correctly depending on whether WithExplicitStart() is set.

Copilot AI mentioned this pull request Dec 12, 2025
Merged
10 tasks
Copy link
Contributor

Copilot AI commented Dec 12, 2025

@danegsta I've opened a new pull request, #13520, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI reviewed Dec 12, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a 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 fixes an issue where BeforeResourceStartedEvent was being fired during resource creation even for resources marked with ExplicitStartupAnnotation. The change ensures this event only fires when a resource is actually starting, not when it's simply being created with the intention to start it manually later.

Key Changes

  • Modified CreateResourceExecutablesAsyncCore to check for ExplicitStartupAnnotation and skip publishing OnResourceStartingContext for such resources
  • Modified CreateContainerAsync to apply the same explicit startup check for container resources
  • Added logic to set the resource state to "NotStarted" when explicit startup is configured

danegsta and others added 2 commits December 11, 2025 16:09
@danegsta
Update src/Aspire.Hosting/Dcp/DcpExecutor.cs
e1ebb1f 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@danegsta
Add unit tests for BeforeResourceStartedEvent with explicit start res…
c8b64bf 
…ources (#13520)

* Initial plan

* Add unit tests for BeforeResourceStartedEvent with explicit start resources

Co-authored-by: danegsta <50252651+danegsta@users.noreply.github.com>

* Refactor tests to use OnBeforeResourceStarted callbacks with Interlocked.Increment

Co-authored-by: danegsta <50252651+danegsta@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: danegsta <50252651+danegsta@users.noreply.github.com>
@danegsta danegsta requested a review from mitchdenny as a code owner December 12, 2025 06:40
JamesNK
JamesNK reviewed Dec 12, 2025
View reviewed changes
JamesNK
JamesNK reviewed Dec 12, 2025
View reviewed changes
JamesNK
JamesNK reviewed Dec 12, 2025
View reviewed changes
@afscrome
Copy link
Contributor

afscrome commented Dec 12, 2025

Looks like the same issue as #13070 and possibly related to #13068

@danegsta
Copy link
Member Author

danegsta commented Dec 12, 2025

Looks like the same issue as #13070 and possibly related to #13068

From manual testing, both those issues would be resolved by this change.

@danegsta
Copy link
Member Author

danegsta commented Dec 12, 2025

@afscrome I may have called you out as "afscrome is going to be really happy to see this PR" when we were discussing this change in a team chat.

JamesNK
JamesNK approved these changes Dec 12, 2025
View reviewed changes
@danegsta danegsta enabled auto-merge (squash) December 12, 2025 23:12
@danegsta danegsta merged commit 00be804 into main Dec 12, 2025
285 of 286 checks passed
@danegsta danegsta deleted the danegsta/onlyBeforeStartOnStart branch December 12, 2025 23:46
@dotnet-policy-service dotnet-policy-service bot added this to the 13.2 milestone Dec 12, 2025
@davidfowl davidfowl added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Dec 13, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@JamesNK JamesNK JamesNK approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl davidfowl is a code owner

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@mitchdenny mitchdenny Awaiting requested review from mitchdenny mitchdenny is a code owner

Assignees

No one assigned

Labels

breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Projects

None yet

Milestone

13.2

Development

Successfully merging this pull request may close these issues.

WithExplicitStart() should block OnBeforeResourceStarted OnBeforeResourceStarted being fired twice on restart

6 participants

@danegsta @afscrome @JamesNK @davidfowl