Fix event order and callback calling during container creation#15503
Fix event order and callback calling during container creation#15503
Conversation
karolz-ms
requested review from
JamesNK,
danegsta,
davidfowl and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15503Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15503" |
|
@afscrome please share your feedback too--I tried to add you as a reviewer, but this new repository has contributor caching issues that prevents me from doing that 😞 |
There was a problem hiding this comment.
Pull request overview
Fixes regressions in Aspire Hosting/DCP startup sequencing introduced by container tunnel changes, ensuring environment/args callbacks run once, run after BeforeResourceStarted, and are isolated on failure.
Changes:
- Adds callback result caching for env/args annotations and resets cache on resource restart.
- Refactors DCP container creation to coordinate tunnel setup with container creation and adjusts lifecycle event publishing (including new connection-string event).
- Updates/extends tests to validate callback invocation order/count and removes quarantine from the proxyless tunnel test via shorter test resource names.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/Dcp/DcpExecutorTests.cs | Adds coverage for callback invocation ordering/count, failure isolation, and mixed tunnel-dependent/independent container startup. |
| tests/Aspire.Hosting.Tests/ContainerTunnelTests.cs | Un-quarantines proxyless tunnel test and shortens test name to avoid invalid resource name length. |
| src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs | Subscribes to and publishes connection-string availability separately from resource-starting. |
| src/Aspire.Hosting/Dcp/Model/ContainerTunnel.cs | Adds UpdatedTunnels state to tunnel proxy resource (non-serialized). |
| src/Aspire.Hosting/Dcp/DcpNameGenerator.cs | Adds stable service-name caching to prevent duplicate service creation for the same endpoint/network. |
| src/Aspire.Hosting/Dcp/DcpExecutorEvents.cs | Introduces OnConnectionStringAvailableContext. |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Major refactor: container creation orchestration, tunnel setup coordination, endpoint-advertisement de-dupe, callback cache reset on restart. |
| src/Aspire.Hosting/Dcp/ContainerCreationContext.cs | New coordination primitives for container creation vs tunnel creation. |
| src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs | Adds optional caching behavior when discovering dependencies by evaluating annotation callbacks once. |
| src/Aspire.Hosting/ApplicationModel/ResourceDependencyDiscoveryMode.cs | Converts discovery mode to [Flags] and adds CacheAnnotationCallbackResults. |
| src/Aspire.Hosting/ApplicationModel/ICallbackResourceAnnotation.cs | New internal interface for “evaluate once + forget cache” callback annotations. |
| src/Aspire.Hosting/ApplicationModel/EnvironmentVariablesConfigurationGatherer.cs | Switches to cached evaluation for environment callbacks. |
| src/Aspire.Hosting/ApplicationModel/EnvironmentCallbackAnnotation.cs | Implements cached evaluation for env callbacks. |
| src/Aspire.Hosting/ApplicationModel/EndpointAnnotation.cs | Adds an internal unique endpoint ID. |
| src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs | Implements cached evaluation for args callbacks. |
| src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs | Switches to cached evaluation for args callbacks. |
src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs
Outdated
Show resolved
Hide resolved
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
The overall approach — caching callback results via EvaluateOnceAsync, coordinating container creation via ContainerCreationContext, and splitting the event ordering so BeforeResourceStarted fires before callbacks — looks sound and addresses the issues in #14954 and #15358. The new tests are well-structured.
However, I found a few problems that should be addressed:
-
Bug: Custom certificate bundles regression —
BuildContainerConfigurationdropped theCustomBundlesFactoriesiteration that existed in the oldCreateContainerAsync. Containers needing custom cert bundles (e.g., Java keystores) will silently break. -
Thread safety:
_appResourcesconcurrent mutation — The tunnel creation task mutates_appResources(viaAddRange/Add) concurrently with non-tunnel containers enumerating it.List<T>is not thread-safe for this. -
Faulted tasks cached permanently — If a callback throws during initial startup, the faulted
Taskis stored and never cleared (onlyForgetCachedResulton restart clears it), making the failure permanent. -
Public API concern —
CacheAnnotationCallbackResultsleaks an internal concern into the publicResourceDependencyDiscoveryModeenum.Recursive = 0in a[Flags]enum is also a footgun. -
Minor:
CountdownEventnot disposed,EndpointIDdead code.
|
@JamesNK thanks for the review! I belive I addressed all your comments. |
|
🎬 CLI E2E Test Recordings — 49 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23510652944 |
Description
This change fixes issues related to environment and invocation argument callback calling and application model events introduced by #14557. It ensures that
Fixes:
#14954
#15358
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: