Skip to content

Don't swallow errors from ExecutionConfigurationBuilder #15475

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.

Sign up for GitHub

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

Open
afscrome wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Don't swallow errors from ExecutionConfigurationBuilder #15475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Aspire.Hosting/Dcp/DcpExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using System.Net;
using System.Net.Sockets;
using System.Runtime.CompilerServices;
using System.Runtime.ExceptionServices;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
Expand Down Expand Up @@ -1870,7 +1871,7 @@ private async Task CreateExecutableAsync(RenderedModelResource er, ILogger resou

if (configuration.Exception is not null)
{
throw new FailedToApplyEnvironmentException();
ExceptionDispatchInfo.Throw(configuration.Exception);
Copy link
Contributor

@karolz-ms karolz-ms Mar 22, 2026

Choose a reason for hiding this comment

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

Why not use InnerException property to remember the original one (contiguration.Exception) when constructing the FailedToApplyEnvironmentException?

I am also not thrilled by this code using ExceptionDispatchInfo--seems like a low-level API that is not really necessary here. Maybe we can eliminate it as part of this PR.

}
Comment on lines 1872 to 1875
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

This change makes configuration.Exception (from ExecutionConfigurationGathererContext.ResolveAsync) propagate instead of being converted into FailedToApplyEnvironmentException. Please add a unit test in DcpExecutorTests that exercises a failing value-provider resolution (e.g., missing endpoint reference) and asserts that the failure is surfaced via error logging / OnResourceFailedToStart rather than being silently swallowed.

Copilot uses AI. Check for mistakes.

// Invoke the debug configuration callback now that endpoints are allocated.
Expand Down Expand Up @@ -2344,7 +2345,11 @@ private async Task CreateContainerAsync(RenderedModelResource cr, ILogger resour
spec.Command = containerResource.Entrypoint;
}

if (failedToApplyRunArgs || configuration.Exception is not null)
if (configuration.Exception is not null)
{
ExceptionDispatchInfo.Throw(configuration.Exception);
}
if (failedToApplyRunArgs)
{
Comment on lines +2350 to 2353
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Same as above for containers: please add coverage for the configuration.Exception path in container startup so regressions don’t reintroduce the "FailedToStart with no logs" behavior (assert an error is logged and the container is marked failed to start).

Suggested change
ExceptionDispatchInfo.Throw(configuration.Exception);
}
if (failedToApplyRunArgs)
{
resourceLogger.LogError(configuration.Exception, "Failed to configure container startup.");
throw new FailedToApplyEnvironmentException();
}
if (failedToApplyRunArgs)
{
resourceLogger.LogError("Failed to apply environment configuration for container startup.");

Copilot uses AI. Check for mistakes.
throw new FailedToApplyEnvironmentException();
}
Expand Down
Loading