Skip to content

P1: Improve portable upgrade swap (temp + atomic replace + rollback) #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kinin-Code-Offical merged 1 commit into from
Dec 22, 2025
Merged

P1: Improve portable upgrade swap (temp + atomic replace + rollback) #39

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/core/selfUpdate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,25 @@ try {
if ($p) { $p.WaitForExit() }
} catch {}

Write-Host "Staging new binary..."
$TempExe = "$TargetExe.tmp"
Copy-Item -Path $NewExe -Destination $TempExe -Force
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Copy-Item operation fails (e.g., due to insufficient disk space or permissions), $TempExe won't be created, but the subsequent Move-Item operations will proceed anyway. This could lead to data loss if $TargetExe is moved to $BackupExe but there's no $TempExe to replace it with. Consider adding error handling or verification after the Copy-Item operation.

Suggested change
Copy-Item -Path $NewExe -Destination $TempExe -Force
try {
Copy-Item -Path $NewExe -Destination $TempExe -Force -ErrorAction Stop
if (-not (Test-Path $TempExe)) {
Write-Host "Staging failed: temporary executable not found at $TempExe"
throw "Staging failed"
}
} catch {
Write-Host "Staging new binary failed, aborting update."
throw
}

Copilot uses AI. Check for mistakes.

Write-Host "Swapping binaries..."
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $NewExe -Destination $TargetExe -Force
try {
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using -ErrorAction SilentlyContinue on the Move-Item that moves $TargetExe to $BackupExe can mask real errors. If this operation fails silently (e.g., due to file locks or permissions), the next Move-Item will attempt to overwrite the active $TargetExe without creating a backup, resulting in potential data loss if the operation fails. Consider removing -ErrorAction SilentlyContinue or at least verifying the backup was created before proceeding.

Suggested change
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
if ((Test-Path $TargetExe) -and -not (Test-Path $BackupExe)) {
throw "Failed to create backup of existing executable."
}

Copilot uses AI. Check for mistakes.
Move-Item -Path $TempExe -Destination $TargetExe -Force
Remove-Item -Path $BackupExe -Force -ErrorAction SilentlyContinue
Comment on lines +329 to +332
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enforce terminating errors for swap failures

In PowerShell, try/catch only handles terminating errors; Move-Item and Copy-Item emit non-terminating errors by default. As written, failures such as a locked target binary or permission issues will not enter the catch block, so the rollback path is skipped and the script still proceeds to Start-Process, potentially leaving the old binary in place or an inconsistent swap state. Consider setting -ErrorAction Stop on the move operations (and the copy if you want a hard fail) so swap failures actually trigger the rollback logic.

Useful? React with 👍 / 👎.

Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backup file is removed immediately after the swap succeeds, but this happens inside the try block before the new version is verified. If the new version fails to start or has issues (line 345), there's no way to recover the backup. Consider moving the backup cleanup outside the try-catch block or after verifying the new version starts successfully.

Copilot uses AI. Check for mistakes.
} catch {
Comment on lines +324 to +333
Copy link

@sourcery-ai sourcery-ai bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Handle failures in the staging copy step consistently with the swap/rollback logic.

Copy-Item runs outside the try block and without -ErrorAction Stop, so failures copying to $TempExe (permissions, disk, locks, etc.) won’t use the same rollback/error handling as the Move-Item failures. Please either move this call into the try block and/or add -ErrorAction Stop so copy failures follow the same rollback path.

Suggested change
Write-Host "Staging new binary..."
$TempExe = "$TargetExe.tmp"
Copy-Item -Path $NewExe -Destination $TempExe -Force
Write-Host "Swapping binaries..."
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $NewExe -Destination $TargetExe -Force
try {
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $TempExe -Destination $TargetExe -Force
Remove-Item -Path $BackupExe -Force -ErrorAction SilentlyContinue
} catch {
Write-Host "Staging new binary..."
$TempExe = "$TargetExe.tmp"
Write-Host "Swapping binaries..."
try {
Copy-Item -Path $NewExe -Destination $TempExe -Force -ErrorAction Stop
Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $TempExe -Destination $TargetExe -Force
Remove-Item -Path $BackupExe -Force -ErrorAction SilentlyContinue
} catch {

Write-Host "Swap failed, attempting rollback..."
if (Test-Path $BackupExe) {
Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction SilentlyContinue
}
if (Test-Path $TempExe) {
Remove-Item -Path $TempExe -Force -ErrorAction SilentlyContinue
Comment on lines +336 to +339
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rollback operations use -ErrorAction SilentlyContinue, which means if the rollback itself fails, the error will be silently suppressed. This could leave the system in a broken state without proper notification. Since this is in the catch block and already handling an error, consider allowing rollback failures to be visible or adding explicit error logging.

Suggested change
Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction SilentlyContinue
}
if (Test-Path $TempExe) {
Remove-Item -Path $TempExe -Force -ErrorAction SilentlyContinue
Move-Item -Path $BackupExe -Destination $TargetExe -Force
}
if (Test-Path $TempExe) {
Remove-Item -Path $TempExe -Force

Copilot uses AI. Check for mistakes.
}
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rollback logic doesn't clean up the $NewExe file after a failed swap. If the swap fails and rollback occurs, the original downloaded file at $NewExe will remain on disk, potentially causing confusion or wasting space. Consider adding cleanup for $NewExe in the catch block.

Suggested change
}
}
if (Test-Path $NewExe) {
Remove-Item -Path $NewExe -Force -ErrorAction SilentlyContinue
}

Copilot uses AI. Check for mistakes.
throw
}
Comment on lines +333 to +342
Copy link

@sourcery-ai sourcery-ai bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Avoid fully silencing rollback failures to aid diagnosis and reliability.

Move-Item is using -ErrorAction SilentlyContinue, which can hide failures to restore the original binary (e.g., due to permissions, locks, or invalid paths). Since rollback is the final safeguard after a failed swap, consider removing SilentlyContinue or at least logging rollback failures so it’s clear when both the update and rollback have failed.

Suggested change
} catch {
Write-Host "Swap failed, attempting rollback..."
if (Test-Path $BackupExe) {
Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction SilentlyContinue
}
if (Test-Path $TempExe) {
Remove-Item -Path $TempExe -Force -ErrorAction SilentlyContinue
}
throw
}
} catch {
Write-Host "Swap failed, attempting rollback..."
if (Test-Path $BackupExe) {
try {
Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction Stop
} catch {
Write-Warning "Rollback failed: could not restore original binary from '$BackupExe' to '$TargetExe'. Error: $_"
}
}
if (Test-Path $TempExe) {
try {
Remove-Item -Path $TempExe -Force -ErrorAction Stop
} catch {
Write-Warning "Rollback cleanup failed: could not remove temporary binary '$TempExe'. Error: $_"
}
}
throw
}


Write-Host "Starting new version..."
Start-Process -FilePath $TargetExe -ArgumentList "--version"
Expand Down