Re-restore integrations when another CLI replaced the assets - #19604
Jonas Flodén (flojon) wants to merge 3 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19604Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19604" |
`obj/aspire-restore.stamp` recorded only the inputs of the last restore performed by this code path, and `CanSkipIntegrationRestore` treated a matching fingerprint as proof that `obj/project.assets.json` still belonged to those inputs. Nothing bound the stamp to the assets, so any other writer of the restore directory could replace them while the stamp kept asserting the old fingerprint. Switching CLI versions is exactly such a writer. A CLI without the stamp logic restores the same synthesized project against its own package versions and leaves the stamp untouched; switching back regenerates a byte-identical `IntegrationRestore.csproj` and therefore the earlier fingerprint, which matches the stale stamp. Restore is skipped and the build runs against the other version's closure — including the TypeScript code generator — while reporting success. Only `rm -rf .aspire` recovered. The stamp now also records a fingerprint of the assets file it produced, and a skip requires the assets on disk to still be the ones it vouches for. A stamp that carries only the inputs is treated as stale, which costs one restore after an upgrade. Fixes microsoft#19603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Binds integration restore stamps to project.assets.json, preventing stale CLI assets from being reused after version switches.
Changes:
- Stores both input and assets fingerprints.
- Validates both fingerprints before skipping restore.
- Adds regression and legacy-stamp tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Aspire.Cli/Projects/PrebuiltAppHostServer.cs |
Adds assets fingerprinting and stamp validation. |
tests/Aspire.Cli.Tests/Projects/PrebuiltAppHostServerTests.cs |
Covers replaced assets, legacy stamps, and missing assets. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
ed270c4 to
ad19858
Compare
Three follow-ups on the assets-fingerprint stamp, found while reproducing microsoft#19603 end to end. `WriteRestoreStampAsync` was gated on `!skipRestore`, but `skipRestore` only says whether the *first* build attempt skipped restore. When `ShouldRetryWithRestore` fires, the retry does restore and rewrites `project.assets.json`, so leaving the stamp describing the assets that restore replaced made it fail its own assets check on the next launch and pay a full restore. Tracked as `restoreRan` instead. `CanSkipIntegrationRestore` reported an unreadable assets file as a replaced one, logging "the assets were replaced" immediately after `TryComputeAssetsFingerprint` logged that it could not read them — two contradictory lines for one event. A null fingerprint now returns without claiming a writer. The stale-stamp check is also split so the log distinguishes a stamp that predates the assets fingerprint from a genuine input change, and the shared helper no longer says "restoring" on the write path, where nothing is. `CanSkipIntegrationRestore_RestoresWhenTheAssetsAreMissing` asked `WriteRestoreState` not to write the assets, but that writes no stamp either -- so the test left neither file and was equivalent to the nothing-restored-yet test above it. It now keeps the stamp and deletes the assets. Note this pins the scenario, not the `File.Exists(assetsPath)` clause: that clause is a cold-start fast path, redundant with the null-fingerprint return, and is documented as such. The remark on `CanSkipIntegrationRestore` said switching back reproduces "a byte-identical project file", which reads as identical between the two CLI versions -- it is not; the pin changes. Reworded to say identical to the one this CLI itself wrote earlier, which is what makes its own stamp match. Verified against the microsoft#19603 repro on a TypeScript AppHost with the released 13.5.1 CLI and the 13.6.0-pr.19577 dogfood build: step 3 logs "restore skipped", rewrites the csproj to 13.6.0-pr, and leaves assets and the built generator at 13.5.1 while reporting success. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reproduced #19603 end to end to confirm this fix actually closes it. It does — One remark worth rewording"Switching back reproduces a byte-identical project file" reads naturally as The repro, confirmedTypeScript AppHost with a project reference, released
Step 3 printed The writer that replaces the assets without refreshing the stamp is simply a CLI Pushed on top (2c03362)Three things that stand independent of the mechanism, plus a wording fix:
Marking ready for review. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
The writer's comment claimed that leaving no stamp costs a restore on the next launch, but its early return leaves whatever stamp an earlier restore wrote sitting on disk. That is the right behaviour rather than a bug: a skip against a surviving stamp still requires the assets on disk to hash to the value it recorded, so the stamp can only be trusted while it describes them, and deleting it would cost a restore for no correctness gain. Only the comment was wrong. The test meant to pin this ran on a fresh workspace, so no stamp existed before the call and the File.Exists assertion would have held even if the writer did nothing at all — it never reached the case its own comment described. It now starts from a completed restore, deletes the assets, and asserts the earlier stamp survives byte-for-byte, which fails on a half-stamp. Dropping the writeAssets escape hatch from WriteRestoreState keeps the helper honest about mirroring a completed restore. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes #19603.
Root cause
obj/aspire-restore.stamprecords only the inputs of the last restore performed byPrebuiltAppHostServer, andCanSkipIntegrationRestoretreats a matching fingerprint as proof thatobj/project.assets.jsonstill corresponds to those inputs. Nothing binds the stamp to the assets, so any other writer of the restore directory can replace them while the stamp keeps asserting the earlier fingerprint.Switching CLI versions is exactly such a writer, which is why the reported failure needs two switches to show up:
13.6.0-prrestores cleanly and writesstamp = A.13.5.1restores the same synthesized project against its own package versions. It rewritesproject.assets.jsonandbin/, and leaves the stamp alone — so the stamp still saysA.13.6.0-prregenerates a byte-identicalIntegrationRestore.csprojand therefore recomputes fingerprintA. It matches the stale stamp, restore is skipped, and the build runs against 13.5.1's closure — includingAspire.Hosting.CodeGeneration.TypeScript.dll— while reporting✅ SDK code restored successfully.That also explains the asymmetry in the issue: going down works because the older CLI has no skip logic and always restores.
Verified against the reported repro with both CLIs installed (
13.5.1and13.6.0-pr.19577.gfa0aea2c). After step 3 the csproj read13.6.0-prwhileproject.assets.jsonand the generator assembly were still13.5.1; deleting just the stamp made the same command restore correctly, confirming the stamp was the only gate.The issue's first suggestion — folding the CLI version into the stamp — does not fix this: the synthesized csproj already encodes the CLI version, so the input fingerprint already differs per version. The problem is that the stamp outlives a foreign restore and the fingerprint returns to a previously stamped value.
Fix
The stamp now records a fingerprint of the assets file it produced alongside the input fingerprint, and a skip requires the assets on disk to still be the ones it vouches for:
XxHash3over the assets stream, consistent with the existing input fingerprint and negligible next to the ~5.6s restore it guards.This also covers the general case the stamp never handled: any external
dotnet restore/dotnet buildagainst the same directory, or a partially cleanedobj/.No warning was added for the version-skew case — with the stamp bound to its assets the state self-corrects on the next command, so there is nothing left to warn about.
Tests
Three new tests in
PrebuiltAppHostServerTests, each verified to fail against the pre-fix logic:CanSkipIntegrationRestore_RestoresWhenAnotherRestoreReplacedTheAssets— the reported scenario: inputs match again but the assets were replaced.CanSkipIntegrationRestore_RestoresWhenTheStampDoesNotVouchForTheAssets— an inputs-only stamp from an earlier CLI.WriteRestoreStampAsync_WritesNoStampWhenTheAssetsAreMissing.The
WriteRestoreStatehelper now writes through the production writer so the tests cannot drift from the stamp format.Full
Aspire.Cli.Testssuite passes (5401 passed, 30 skipped Windows-only, 0 failed).🤖 Generated with Claude Code