You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Isolate crashed Aspire probe from certificate preparation - #7566
The crashed-process regression can spend its startup budget in Aspire preparation before DCP launches the executable. Disable custom certificate trust for this probe, which makes no TLS connections, and remove the unused my-secret parameter along with the other unrelated AppHost resources. Keep the 60-second bound, the never-healthy check, the required InvalidOperationException, and both live/retained process-output assertions.
Aspire 13.6.4 defaults executable trust to Append; CertificateTrustExecutionConfigurationGatherer returns early for None, and ExecutableConfigurationResolver does not emit PEM certificate configuration for that scope. This removes an unnecessary pre-launch path from this regression. The original intermittent Ubuntu stall did not reproduce locally, so this is fixture isolation rather than a proven upstream deadlock diagnosis.
Validation: the targeted CrashedExecutableLogTests filter passed 2/2 in Release/net10.0 on macOS arm64 and in the Linux arm64 .NET SDK 10.0.403 container. The macOS baseline also passed 2/2. Ubuntu-hosted and Windows validation still need CI; no production API or generator output changed.
Review was skipped as selected files did not have any reviewable changes.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 1bfbe00f-945e-4c9e-b871-cdc168671d0b
Commits
Reviewing files that changed from the base of the PR and between 4cf89bc and 55736ee.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
The crashed-executable test fixture now excludes my-secret and sets the probe executable’s certificate trust scope to None.
Changes: Crashed-executable test setup
Layer / File(s)
Summary
Configure the test fixture tests/TUnit.Aspire.Tests/CrashedExecutableLogTests.cs
The fixture excludes my-secret and updates its comment about excluded resources. The probe executable sets CertificateTrustScope.None; its comment states that it makes no TLS connections. The file adds the Aspire.Hosting.ApplicationModel import.
thomhurst/TUnit#7283: Introduced the crashed-executable tests and runtime-instance log behavior that this change’s fixture modifies.
Merge Risk: Minimal · previous review 4cf89
The change is limited to test-fixture isolation, with no established production or test correctness regression in the supplied evidence.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Linked Issues check
Issue #7555 requires a reliable fixture that observes the nonzero dotnet exit, preserves InvalidOperationException, the live crashed-process marker assertion, and the retained-log assertion, and v…
Run both CrashedExecutableLogTests tests on Ubuntu in Release/net10.0 and provide successful CI evidence. Confirm that the run observes the nonzero process exit and InvalidOperationException, while the live and retained output assertion…
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: isolating the crashed Aspire probe from certificate preparation.
Out of Scope Changes check
The reviewed changes are limited to CrashedExecutableLogTests.cs. They remove the unresolved my-secret and unrelated resources, and disable certificate preparation for the probe. These changes dir…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Out of Scope Changes check
Explanation
The reviewed changes are limited to CrashedExecutableLogTests.cs. They remove the unresolved my-secret and unrelated resources, and disable certificate preparation for the probe. These changes directly isolate the fixture described by issue #7555. No unrelated production or test behavior is shown.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
Full details: Linked Issues check
Explanation
Issue #7555 requires a reliable fixture that observes the nonzero dotnet exit, preserves InvalidOperationException, the live crashed-process marker assertion, and the retained-log assertion, and validates both tests on Ubuntu. The diff removes my-secret and unrelated resources, sets CertificateTrustScope.None, keeps the 60-second timeout and never-healthy check, and preserves both output assertions. However, the PR description states that Ubuntu-hosted validation is still pending. The available evidence therefore does not satisfy the Ubuntu validation requirement.
Resolution
Run both CrashedExecutableLogTests tests on Ubuntu in Release/net10.0 and provide successful CI evidence. Confirm that the run observes the nonzero process exit and InvalidOperationException, while the live and retained output assertions pass. Preserve Windows and macOS validation.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the probe at dawn. No secret resource lingers on. Trust scope rests at None today. The logs still mark the process way. I nibble clover, then I hop away.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 683f3208-b5e1-4bd0-9820-dabc8a5f5ccd
Commits
Reviewing files that changed from the base of the PR and between 8d0cffe and 4cf89bc.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
Test-only change that isolates the crashed-executable fixture: it removes the unused my-secret parameter (the name matches the AppHost's AddParameter("my-secret", ...)) and sets WithCertificateTrustScope(CertificateTrustScope.None) on the probe. The 60s bound, never-healthy check, expected exception and output assertions are unchanged. ResourcesToRemove only affects this fixture, so WaitForHealthyReproductionTests is unaffected.
Two notes, neither blocking:
The PR description says this is fixture isolation, not a proven diagnosis of the Ubuntu stall. If the flake comes back, the next step is to capture Aspire/DCP logs during the startup window. Changing the fixture again won't tell you more.
The comment on the trust scope explains the reason well, so a later reader won't re-enable it by accident.
I couldn't run the tests here. They need CI on Ubuntu and Windows, as the description says.
Marked ready after Claude and Codex approved head 4cf89bc; all review surfaces have no blocking findings or unresolved threads. The head is unchanged. Waiting for the ready-for-review Claude run 41787174229 and CI. Read its verdict before arming auto-merge, as required by the repository merge gate.
Test-only change to CrashedExecutableLogTests. Setting CertificateTrustScope.None on the probe executable and removing the unused my-secret parameter both cut pre-launch work. The probe makes no TLS connections and never uses the parameter. The 60s bound, never-healthy check, InvalidOperationException and live/retained output assertions are unchanged.
ResourcesToRemove relies on hard-coded names from the AppHost. A rename would silently stop removing a resource. This was already the case, so it isn't a blocker.
No production code is affected, so no generator, AOT or public API concerns apply.
The change is small and test-only. It does two things in CrashedExecutableLogTests:
It sets WithCertificateTrustScope(CertificateTrustScope.None) on the crashing probe, so the 60s startup budget no longer includes certificate preparation.
It adds my-secret to ResourcesToRemove, so the probe's AppHost doesn't carry an unresolved parameter.
I checked that my-secret is still declared in TUnit.Aspire.Tests.AppHost/Program.cs. Other tests still use it (WaitForHealthyReproductionTests). Removing it per fixture is therefore the right scope, and the shared AppHost stays intact.
The 60s bound, the never-healthy check, the InvalidOperationException expectation and the live/retained output assertions are all untouched. The PR description is also upfront that this is fixture isolation rather than a proven root-cause fix for #7555. The explanatory comments are accurate.
Two non-blocking notes:
Since the original stall never reproduced locally, watch CI on Ubuntu and Windows. If it recurs, the next suspect is DCP or executable launch timing, not certificate trust.
I haven't built or run the tests, so my confidence comes from reading the diff only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Inactive
The crashed-process regression can spend its startup budget in Aspire preparation before DCP launches the executable. Disable custom certificate trust for this probe, which makes no TLS connections, and remove the unused
my-secretparameter along with the other unrelated AppHost resources. Keep the 60-second bound, the never-healthy check, the requiredInvalidOperationException, and both live/retained process-output assertions.Aspire 13.6.4 defaults executable trust to
Append;CertificateTrustExecutionConfigurationGathererreturns early forNone, andExecutableConfigurationResolverdoes not emit PEM certificate configuration for that scope. This removes an unnecessary pre-launch path from this regression. The original intermittent Ubuntu stall did not reproduce locally, so this is fixture isolation rather than a proven upstream deadlock diagnosis.Validation: the targeted
CrashedExecutableLogTestsfilter passed 2/2 in Release/net10.0 on macOS arm64 and in the Linux arm64 .NET SDK 10.0.403 container. The macOS baseline also passed 2/2. Ubuntu-hosted and Windows validation still need CI; no production API or generator output changed.Closes #7555
Summary by CodeRabbit