Repository navigation
Isolate source AppHost deployment state - #19709
Mitch Denny (mitchdenny) merged 18 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces support for polyglot/source-file AppHost deployment-state identity and legacy-state migration, including concurrency-safe persistence and expanded APIs/tests to validate migration behavior.
Changes:
- Add migration-aware state loading/saving in
FileDeploymentStateManager(canonical vs legacy paths, migration metadata, cross-process locking, claimed-section tracking). - Extend deployment-state APIs to support “current-only” acquisitions and section-level original-data tracking for safer merges.
- Update AppHost SHA/identity behavior and publishing-time state loading to handle polyglot AppHosts; add comprehensive migration tests.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/Publishing/DeploymentStateManagerTests.cs | Adds tests for explicit-null persistence and many legacy→canonical migration/concurrency scenarios. |
| tests/Aspire.Hosting.Tests/DistributedApplicationBuilderTests.cs | Adds tests for polyglot AppHost identity hashing and legacy deployment-state loading/clearing behavior. |
| src/Aspire.Hosting/Pipelines/Internal/UserSecretsDeploymentStateManager.cs | Updates override signature to match new base persistence contract. |
| src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs | Implements migration metadata, canonical/legacy path selection, section-delta writes, and file locking. |
| src/Aspire.Hosting/Pipelines/Internal/DeploymentStateManagerBase.cs | Adds section-aware persistence contract, “current-only” acquisition path, and storage-load/clear extensibility. |
| src/Aspire.Hosting/Pipelines/IDeploymentStateManager.cs | Adds AcquireCurrentSectionAsync API surface (currently via default interface method). |
| src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs | Updates --clear-cache behavior to account for migration metadata files. |
| src/Aspire.Hosting/Pipelines/DeploymentStateSection.cs | Adds OriginalData tracking to support safer merge-on-save behavior. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Computes legacy vs canonical AppHost identities for polyglot hosts and loads “effective” deployment state. |
| src/Aspire.Hosting/Aspire.Hosting.csproj | Links FileLock.cs into the project for cross-process state coordination. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19709Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19709" |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs:285
- A full save can receive the flattened shape supported by
SaveStateAsync(the existingBackwardCompatibility_LoadsStateWithoutMetadatatest passes keys such asParameters:param1). Assigning that object directly makes the migration sidecar's authoritativeCurrentStateflat, so subsequentAcquireSectionAsync("Parameters")cannot navigate to the saved value. Normalize the input through the same flatten/unflatten storage round-trip before persisting it.
_currentState = state.DeepClone().AsObject();
tests/Aspire.Hosting.Tests/DistributedApplicationBuilderTests.cs:465
- Constructing this source-file builder acquires the canonical lock, which creates the canonical deployment directory even when no canonical state exists. This test only removes
legacyDirectory, so every run leaves an empty source-specific directory under the real~/.aspire/deployments. Track the builder'sAppHost:PathSha256directory and remove it infinally, as the following migration-loading tests do.
var builder = (DistributedApplicationBuilder)DistributedApplication.CreateBuilder(options);
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tests/Aspire.Hosting.Tests/DistributedApplicationBuilderTests.cs:481
- This cleanup removes only the legacy directory. Builder construction now acquires the canonical lock, and
FileLockcreates that lock directory even when no canonical state exists, so this test leaves an empty source-specific directory under the real~/.aspire/deploymentson every run. Capture the builder's current SHA and remove its empty deployment directory infinally, as the following tests do.
if (Directory.Exists(legacyDirectory) && !Directory.EnumerateFileSystemEntries(legacyDirectory).Any())
{
Directory.Delete(legacyDirectory);
}
projectDirectory.Delete(recursive: true);
src/Aspire.Hosting/Pipelines/Internal/DeploymentStateManagerBase.cs:150
- A full-state save leaves the base
_statecache unchanged. If this manager loaded state earlier, the nextAcquireSectionAsyncreturns the pre-replacement snapshot; the new full-replacement flow can then treat an explicit post-replacement save as unchanged against stale data. Reload the successfully saved state so same-instance reads observe both the replacement and the backing store's normalized shape.
await SaveStateToStorageAsync(state, sectionName: null, originalSectionData: null, cancellationToken).ConfigureAwait(false);
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
LoadDeploymentState now computes state paths inside the try/catch so an environment name containing characters outside [a-zA-Z0-9_-] degrades to skipping the best-effort load instead of throwing ArgumentException from builder construction in publish mode. FileDeploymentStateManager only locks and reads legacy state when the legacy file exists. This avoids manufacturing a phantom legacy directory and lock file for identities that never had legacy state (e.g. brand-new source AppHosts on a clean machine) and avoids re-coupling sibling AppHosts that share the legacy identity on that shared lock. Adds regression tests that fail without each fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The test fails in PR CI and is tracked by microsoft#19708, which has a separate fix in progress. Mark it [ActiveIssue] so it does not block unrelated PRs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Hosting/DistributedApplicationBuilder.cs:322
- The deployment-state identity canonicalizes casing but not symbolic links, so the same source AppHost gets different state directories when launched through a symlink versus its physical path (for example
/var/.../apphost.tsand/private/var/.../apphost.tson macOS). The CLI passesappHostFile.FullNameunchanged, while the existing workload/backchannel identities explicitly callResolveSymlinksto avoid this split. Resolve symlinks before filesystem-case normalization so path aliases do not lose access to canonical deployment state.
var appHostIdentityPath = isSourceFileAppHost
? PathNormalizer.ResolveToFilesystemPath(Path.GetFullPath(appHostFilePath!))
: AppHostPath;
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…t#19733 PR microsoft#19733 quarantines this test with [QuarantinedTest] linked to microsoft#19708, which preserves quarantine-workflow coverage until the root cause is fixed in microsoft#19731. Our earlier [ActiveIssue] change skipped the test everywhere (including the quarantine workflow) and touched the same method, causing a merge conflict. Remove it so microsoft#19733 owns disabling this test and this branch leaves the file unchanged from base. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/Aspire.Hosting/DistributedApplicationBuilder.cs:322
- This hashes the lexical path of a symlink instead of the source file it resolves to. On Linux
ResolveToFilesystemPathis a no-op, and on macOS/Windows it only canonicalizes casing, so launching the same AppHost throughreal/apphost.tsandalias/apphost.tscreates different deployment-state identities and loses access to the prior canonical state. Resolve symlinks before canonicalizing casing, consistent withAuxiliaryBackchannelService.cs:231.
This issue also appears on line 323 of the same file.
? PathNormalizer.ResolveToFilesystemPath(Path.GetFullPath(appHostFilePath!))
src/Aspire.Hosting/DistributedApplicationBuilder.cs:327
- This new identity is used only for local naming and lookup, not for security, so adding another SHA-256 computation is inconsistent with the repository's non-cryptographic identity pattern (for example
BackchannelConstants.cs:484-498usesXxHash3for the same purpose). UseSystem.IO.Hashing.XxHash3for the new identity and reserve SHA-256 only for the legacy keys that require compatibility.
var deploymentStatePathShaBytes = SHA256.HashData(Encoding.UTF8.GetBytes(normalizedAppHostIdentityPath));
deploymentStatePathSha = Convert.ToHexString(deploymentStatePathShaBytes);
src/Aspire.Hosting/DistributedApplicationBuilder.cs:325
- Lowercasing the entire resolved source path on Windows collapses distinct files in a case-sensitive NTFS directory (Windows supports per-directory case sensitivity).
ResolveToFilesystemPathalready converges casing aliases on ordinary case-insensitive volumes, so the source-file segments must retain their resolved casing; otherwise siblingapphost.tsandAppHost.tsfiles receive the same deployment-state identity and defeat this isolation change.
var normalizedAppHostIdentityPath = isSourceFileAppHost && !OperatingSystem.IsWindows()
? appHostIdentityPath
: appHostIdentityPath.ToLowerInvariant();
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs:286
mergedSectionis built from the base manager's cached_state, butSaveSectionAsyncreplaces that cache with the caller's section before this class performs its three-way merge. This makes subsequent reads from the same manager disagree with persisted state. For example, acquire a current-only parent containing canonical childfirstwhile legacy fallback also hassecond, save that section unchanged, then call ordinaryAcquireSectionAsync: storage still includes legacysecond, but the cached parent no longer does, sosecondis omitted until restart. Build the read view from the freshly merged_legacyState/_currentStatefields instead of the stale base cache.
var mergedSection = base.GetSectionState(state, sectionName, includeLegacyState)?.DeepClone();
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
Approving. Reviewed the deployment-state isolation change in isolation; the source-file identity + immutable legacy read-through migration is sound and well-tested (concurrency, tombstones, partial/repeated migration, malformed metadata).
Two edge cases were found and fixed on the branch during review, each with a regression test verified to fail without the fix:
LoadDeploymentStatecomputed state paths outside its try/catch, so an environment name outside[a-zA-Z0-9_-]threwArgumentExceptionfrom builder construction in publish mode. Path resolution is now inside the guarded block.- Legacy state reads acquired the cross-process lock without a
File.Existsguard, manufacturing phantom legacy directories/locks for brand-new source AppHosts and re-coupling siblings on a shared lock. Reads are now gated on legacy-file existence (safe, since legacy state is immutable in this model).
Also reconciled the SelfUpdateChannelPersistenceTests disable with #19733 (now merged), and merged main. CI is fully green (385 pass / 0 fail).
Follow-up filed: #19734 to update the deployment-state-caching docs for the new source-AppHost layout.
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
Source-file AppHosts in the same directory currently share a directory-scoped deployment-state identity. That allows one AppHost to enumerate, update, or clear state that belongs to a sibling AppHost.
This change gives source-file AppHosts a source-specific deployment-state identity while preserving safe access to legacy state during migration. Legacy storage remains immutable because its ownership is ambiguous. Each source AppHost records an atomic migration snapshot, canonical deltas, and tombstones in its own sidecar so interrupted writes cannot revive deleted state. Parent-section saves use three-way leaf merging, preserving independent concurrent changes and preventing unchanged sibling descendants from being adopted.
The deployment-state API also gains
AcquireCurrentSectionAsync, which lets deployment targets enumerate only canonical state while ordinary reads retain migration fallback. This is a prerequisite for the Azure Container Apps Sandboxes work in #19008.User-facing usage
Deployment-state consumers that must exclude legacy-only entries can acquire the current AppHost's canonical section:
Validation includes focused coverage for source identity, partial and repeated migration, broad parent saves, concurrent writers, tombstones, clear behavior, malformed metadata, interrupted compatibility writes, and early configuration loading.
Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?