Fix install script swallowing file-lock errors and reporting false success#15419
Fix install script swallowing file-lock errors and reporting false success#15419
Conversation
… success messages When aspire.exe was locked by another process, Move-Item in Backup-ExistingCliExecutable failed as a non-terminating error and execution continued, causing Expand-Archive to also fail silently and the success message to be displayed regardless. - Wrap Move-Item in Backup-ExistingCliExecutable with try/catch + -ErrorAction Stop in both get-aspire-cli.ps1 and get-aspire-cli-pr.ps1, throwing a clear actionable message when the backup fails - Add -ErrorAction Stop to Expand-Archive in both scripts so extraction failures are caught by the existing catch block - Add exit-code check for tar in get-aspire-cli.ps1 (pr variant already had it) Co-authored-by: radical <1472+radical@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15419Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15419" |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves robustness of the Aspire CLI install PowerShell scripts by converting previously non-terminating file operation failures (notably when aspire.exe is locked) into terminating errors that are properly caught and reported, preventing false “success” output.
Changes:
- Make backup/restore of existing CLI executable fail fast (
-ErrorAction Stop) and surface an actionable error when backup can’t be created. - Ensure archive extraction failures are terminating (
Expand-Archive -ErrorAction Stop) so the existingtry/catchlogic runs (including backup restore). - Add Unix
tarexit-code validation inget-aspire-cli.ps1(PR variant already had this behavior).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| eng/scripts/get-aspire-cli.ps1 | Harden backup/restore + extraction paths to avoid swallowed errors and false success; add tar exit-code check. |
| eng/scripts/get-aspire-cli-pr.ps1 | Align PR installer with the same backup/restore + extraction error handling improvements. |
| Move-Item -Path $BackupPath -Destination $TargetExePath -Force -ErrorAction Stop | ||
| } |
There was a problem hiding this comment.
Restore-CliExecutableFromBackup now uses Move-Item -ErrorAction Stop, which will throw if the restore fails (e.g., the target is still locked). In Expand-AspireCliArchive the restore call is not wrapped, so a restore failure will short-circuit the catch block and can mask the original extraction failure. Consider catching restore errors inside Expand-AspireCliArchive and either (a) include both the original extraction error and the restore error in the final thrown message, or (b) log the restore error and still rethrow the original extraction failure.
| } | ||
|
|
||
| Move-Item -Path $BackupPath -Destination $TargetExePath -Force | ||
| Move-Item -Path $BackupPath -Destination $TargetExePath -Force -ErrorAction Stop |
There was a problem hiding this comment.
Restore-CliExecutableFromBackup now uses Move-Item -ErrorAction Stop, which will throw if the restore fails (e.g., the target is still locked). In Expand-AspireCliArchive the restore call in the catch block isn’t wrapped, so a restore failure will abort the catch block and can hide the original extraction error. Consider handling restore failures inside Expand-AspireCliArchive so the final error surfaced to the user retains the primary failure cause while still reporting any restore failure.
| Move-Item -Path $BackupPath -Destination $TargetExePath -Force -ErrorAction Stop | |
| try { | |
| Move-Item -Path $BackupPath -Destination $TargetExePath -Force -ErrorAction Stop | |
| } | |
| catch { | |
| # Do not rethrow here: this runs during error recovery and should not mask the original failure. | |
| Write-Message "Failed to restore CLI from backup '$BackupPath' to '$TargetExePath'. The backup file may be in use or the target may be locked. Error: $($_.Exception.Message)" -Level Warning | |
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # Rename existing executable to .old.[timestamp] | ||
| Move-Item -Path $TargetExePath -Destination $backupPath -Force | ||
| try { | ||
| Move-Item -Path $TargetExePath -Destination $backupPath -Force -ErrorAction Stop | ||
| } | ||
| catch { | ||
| throw "Failed to back up existing CLI at '$TargetExePath'. The file may be in use by another process. Please close any running Aspire CLI instances and try again. Error: $($_.Exception.Message)" |
There was a problem hiding this comment.
Backup-ExistingCliExecutable now correctly fails when Move-Item can’t back up a locked executable, but the surrounding wording (“allows installation to proceed even when the CLI is running” / “file can be renamed”) can be misleading because rename may still fail depending on the handle’s sharing mode. Consider updating the nearby comment/message to clarify that the script attempts to rename and will stop with an actionable error if the file can’t be moved (e.g., locked by another process).
| & tar -xzf $ArchiveFile | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "tar command failed with exit code $LASTEXITCODE" |
There was a problem hiding this comment.
The thrown tar failure message currently only includes the exit code. Including $ArchiveFile (and ideally tar’s stderr/stdout) would make failures much easier to diagnose, especially when multiple archives/paths are involved.
| & tar -xzf $ArchiveFile | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "tar command failed with exit code $LASTEXITCODE" | |
| $tarOutput = & tar -xzf $ArchiveFile 2>&1 | |
| $tarExitCode = $LASTEXITCODE | |
| if ($tarExitCode -ne 0) { | |
| $message = "tar command failed for archive '$ArchiveFile' with exit code $tarExitCode." | |
| if ($tarOutput) { | |
| $message += " Output:`n$tarOutput" | |
| } | |
| throw $message |
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
🎬 CLI E2E Test Recordings — 52 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23328308198 |
When
aspire.exewas held open by another process during install,Move-IteminBackup-ExistingCliExecutableemitted a non-terminating error — PowerShell wrote to the error stream but kept executing. The backup path was pre-assigned before theMove-Itemcall, so the function returned a non-null path despite never creating the backup file.Expand-Archivethen tried to overwrite the still-locked binary, also failed silently, and thecatchblock was never triggered — causing the script to print the success message against a failed install.Changes (
get-aspire-cli.ps1andget-aspire-cli-pr.ps1)Backup-ExistingCliExecutable: WrapMove-Itemwith-ErrorAction Stopinside atry/catch. On failure, throw an actionable message:Expand-AspireCliArchive(Windows): Add-ErrorAction StoptoExpand-Archiveso extraction failures are terminating and caught by the existingcatchblock (which also restores the backup).Expand-AspireCliArchive(Unix,get-aspire-cli.ps1only): Add$LASTEXITCODEcheck aftertar— the PR-variant already had this.Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue:Original prompt
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.