Fix PackagedApp registration when switching build layouts - #11372
Sergio Pedri (Sergio0694) wants to merge 9 commits into
Conversation
Verify the current-user registration before activation, replace stale development registrations while preserving application data, and fail safely if the requested layout cannot be registered. Add regression coverage and update localized diagnostics and package documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The registration-to-activation sequence has an unresolved cross-process race in sensitive per-user Windows package state.
Pull request overview
Fixes PackagedApp layout switching by verifying Windows registration before AUMID activation.
Changes:
- Re-registers stale development layouts while preserving application data.
- Adds diagnostics, localization resources, documentation, API baselines, and regression tests.
File summaries
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/PackageDeployerTests.cs | Tests registration and failure scenarios. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.zh-Hant.xlf | Adds Traditional Chinese localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.zh-Hans.xlf | Adds Simplified Chinese localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.tr.xlf | Adds Turkish localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.ru.xlf | Adds Russian localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.pt-BR.xlf | Adds Brazilian Portuguese localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.pl.xlf | Adds Polish localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.ko.xlf | Adds Korean localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.ja.xlf | Adds Japanese localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.it.xlf | Adds Italian localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.fr.xlf | Adds French localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.es.xlf | Adds Spanish localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.de.xlf | Adds German localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/xlf/ExtensionResources.cs.xlf | Adds Czech localization entries. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/Resources/ExtensionResources.resx | Defines new registration diagnostics. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/RegisteredPackageInfo.cs | Models installed package registration details. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/PackageDeployer.cs | Verifies and replaces stale registrations. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/PACKAGE.md | Documents layout-switching behavior. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/InternalAPI/Windows/InternalAPI.Unshipped.txt | Updates Windows internal API baseline. |
| src/Platform/Microsoft.Testing.Extensions.PackagedApp/InternalAPI/InternalAPI.Unshipped.txt | Tracks shared internal APIs. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Hold a cancellable per-user, per-package-family file lease across registration, handoff creation, and AUMID activation. Add contention, cancellation, thread-switch, failure, and cross-process regression coverage, with related diagnostics and documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Windows package lifecycle and cross-process synchronization changes require human review, with removal-state and cancellation handling still unresolved.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Platform/Microsoft.Testing.Extensions.PackagedApp/PackageDeployer.cs:77
ExtendedErrorCodeonly reports whether the removal operation completed successfully;DeploymentResult.IsRegisteredreports whether the package remains registered. If removal returnsS_OKbut leaves the package registered, this code retries registration against the stale layout and later reports a location mismatch instead of the removal failure. Treat either a still-registered result or a negative HRESULT as removal failure before retrying.
src/Platform/Microsoft.Testing.Extensions.PackagedApp/PackageDeployer.cs:105- Both registration queries have a cancellation gap. If the token is canceled while
findRegisteredPackages()is running and that query returns an ambiguous, missing, or non-development registration, the following branch throws a wrapped deployment error before the later cancellation check runs. Check the token immediately after this query and after the retry query at line 132 so cancellation consistently propagates asOperationCanceledException.
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
Validate that stale registrations are actually removed and finish restoring the requested layout before propagating cancellation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Completed an extensive lifecycle, concurrency, API-baseline, localization, and test-quality review. I found and fixed three additional correctness gaps in commit 98683104c:
- A canceled token could be missed immediately after either package-registration query, causing a location or registration error to win over cancellation and potentially starting destructive removal.
- A completed removal operation could still report the package as registered; replacement registration now refuses to proceed unless
DeploymentResult.IsRegisteredconfirms removal. - Cancellation arriving after stale-registration removal could leave the package intentionally unregistered. Once removal starts successfully, the code now restores and verifies the requested layout before propagating cancellation.
The original cross-process registration-to-activation race is correctly addressed by the per-user, package-family file lease. I found no remaining actionable defects in the lock identity, lease lifetime, activation ordering, API baselines, generated XLF changes, or the changed tests.
CI build 1602057 had no product test failures: both Windows legs stopped before build/test because the hosted agents lacked the expected LocalDumps registry key, while the macOS test step passed before the job hit its worker timeout. A fresh build, 1602159, was queued by the follow-up commit.
Review coverage
- Registration state transitions, same-version layout replacement, and non-development safeguards
- Cross-process locking, case normalization, cancellation, failure cleanup, and activation ordering
- Windows deployment-result semantics and post-removal consistency
- All four package target frameworks and internal API baselines
- Resource diagnostics, generated localization files, package documentation, and UTF-8 BOM requirements
- Changed-test isolation, deterministic synchronization, assertion relevance, negative paths, and cancellation boundaries
- Azure Pipelines failure classification and correlation against the base-branch build
This comment has been minimized.
This comment has been minimized.
Hold a read-shared manifest lease throughout registration and activation, and use the same parsed identity for locking, verification, and the AUMID. Preserve safety-validation diagnostics by wrapping only deployment operation failures. Confirm removal by querying current-user registrations: native RemovePackageAsync can report IsRegistered=true even after successful unregistration. Preserve the existing cancellation recovery and add manifest-race and diagnostic regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fresh Windows agents may not have the WER LocalDumps key. Create it only when absent before writing DumpFolder and DumpCount, preserving existing values and per-application settings. This fixes the pre-build Windows setup failure in build 1602221. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-process synchronization, destructive registration replacement, cancellation recovery, and an unresolved misleading failure path require human validation.
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 1
- Review effort level: Balanced
Limit the removal callback to the native deployment operation and verify current-user registration separately in the transaction. Preserve original lookup failures and cancellation recovery, and add regression coverage distinguishing removal errors from post-removal query errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
OS-dependent registration mutation, cross-process locking, and cancellation recovery require final human validation.
Review details
Suppressed comments (1)
src/Platform/Microsoft.Testing.Extensions.PackagedApp/PackageDeployer.cs:193
- A native removal failure is wrapped with
PackagedAppRegistrationFailed, so an error such as “package is in use” is reported as “Failed to register” with Developer Mode remediation. This misidentifies the failed operation and can send users toward an unrelated fix. Use a removal-specific resource here, preserve the original exception as the inner exception, and regenerate the XLF files from the RESX entry.
catch (Exception ex) when (ex is not OperationCanceledException)
{
throw CreateDeploymentFailureException(manifestPath, ex);
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Preliminary test-triage analysis — Azure Pipelines build 1603070 Analysis mode: early (Windows Release build leg failed; other legs in the aggregate build may still be running). Failing check: Failure signature: Findings:
Confidence: Low-to-moderate that this is a code defect. The evidence to date looks more consistent with an environment-specific timing margin issue (close to the 9s bound, isolated to one leg) than a deterministic regression, but this cannot be confirmed until the full build evidence and any retries are available. Next step: Await the completed aggregate build. If this test fails again on Windows Release net462 (or elsewhere) with a similar or worse margin, or if history across subsequent builds shows a recurring pattern, that would support raising the assertion bound or investigating the Note: other build legs may still be running; this preliminary analysis may be superseded by a final resolution comment once the aggregate build completes.
|
Summary
Fixes PackagedApp activating an older registered layout instead of the requested build when both layouts have the same package identity and version.
DeploymentResult.IsRegisteredalone does not guarantee that Windows registered the requested directory.PreserveApplicationData, register the requested layout again, and verify the result. Never remove a conflicting non-development registration or activate an unverified layout.Related to #9933.
Validation
All original validation above was performed by the parent session on exactly this fix. Temporary Windows registrations have been removed and the parent's worktree is clean.