-
Notifications
You must be signed in to change notification settings - Fork 752
Fix duplicate OnBeforeResourceStarted event on container restart #13385
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
- Remove duplicate OnResourceStartingContext publish in StartResourceAsync for containers - CreateContainerAsync already publishes this event during both initial creation and restart - Executables are unaffected as they follow a different pattern - Add test to verify the fix for containers - Add test to verify executables still work correctly Co-authored-by: davidfowl <[email protected]>
- Use consistent DistributedApplicationOptions in both tests - Both tests now follow the same pattern for builder initialization Co-authored-by: davidfowl <[email protected]>
| c.Spec.Start = true; | ||
|
|
||
| await _executorEvents.PublishAsync(new OnResourceStartingContext(cancellationToken, resourceType, appResource.ModelResource, appResource.DcpResourceName)).ConfigureAwait(false); | ||
| // OnResourceStartingContext will be published by CreateContainerAsync |
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 Is this correct? What will not firing OnResourceStartingContext break?
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, this is correct. Both the initial creation path and the restart path go through CreateContainerAsync, which publishes OnResourceStartingContext at line 1918.
Initial creation: CreateContainersAsync → CreateContainerAsyncCore → CreateContainerAsync → publishes event
Restart: StartResourceAsync → CreateContainerAsync → publishes event
The only subscriber is ApplicationOrchestrator.OnResourceStarting, which sets the state to "Starting", updates container properties, and publishes BeforeResourceStartedEvent. This all still happens correctly with the change.
The bug was that on restart, StartResourceAsync was publishing the event AND then CreateContainerAsync was publishing it again, causing the duplicate. The test ContainerRestarted_OnResourceStartingContextFiredOnce verifies this fix works correctly.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13385Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13385" |
|
@copilot these added test should be using the WithTestAndResourceLogging API |
- Add ITestOutputHelper parameter to DcpExecutorTests class constructor - Call WithTestAndResourceLogging in both new tests for better diagnostics - Add using statement for Aspire.Hosting.Utils namespace Co-authored-by: davidfowl <[email protected]>
Summary
Fixes #7733 -
OnBeforeResourceStartedevent firing twice on container restartProblem
When restarting a container resource via the dashboard, the
OnBeforeResourceStartedevent was being fired twice instead of once. This was causing duplicate log messages and potentially triggering callbacks twice.Root Cause
In
DcpExecutor.StartResourceAsync, when restarting a container:OnResourceStartingContextCreateContainerAsyncCreateContainerAsyncpublishesOnResourceStartingContextagainApplicationOrchestrator.OnResourceStartinghandler publishesBeforeResourceStartedEventfor each contextSolution
Remove the duplicate
OnResourceStartingContextpublish fromStartResourceAsyncfor containers.CreateContainerAsyncalready publishes this event during both initial creation and restart, so the duplicate was unnecessary.Why Executables Are Unaffected
Executables follow a different pattern:
CreateResourceExecutablesAsyncCorepublishes the event (line 1506) before callingCreateExecutableAsyncStartResourceAsyncpublishes the event before callingCreateExecutableAsyncCreateExecutableAsyncnever publishes the eventTherefore, removing the event publish from
StartResourceAsyncfor executables would break initial startup.Changes Made
OnResourceStartingContextpublish inStartResourceAsyncfor containersContainerRestarted_OnResourceStartingContextFiredOnce- Verifies containers only fire the event once on restartResourceRestarted_OnResourceStartingContextFiredOnce- Verifies executables still work correctlyWithTestAndResourceLoggingfor better diagnosticsTesting
ContainerRestarted_OnResourceStartingContextFiredOncetest - reproduces the bug before the fix, passes afterResourceRestarted_OnResourceStartingContextFiredOncetest - ensures executables still work correctlyDcpExecutorTestspassApplicationOrchestratorTestspassWithTestAndResourceLoggingfor diagnosticsOriginal prompt
OnBeforeResourceStartedbeing fired twice on restart #13068💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.