P1: Improve portable upgrade swap (temp + atomic replace + rollback)#39
P1: Improve portable upgrade swap (temp + atomic replace + rollback)#39Kinin-Code-Offical merged 1 commit intomainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideImplements a safer portable self-update flow for the Windows binary by staging the new executable as a temp file in the target directory, performing an atomic-style swap with backup and cleanup, and adding rollback handling if the swap fails. Sequence diagram for portable self-update swap with staging and rollbacksequenceDiagram
actor User
participant SelfUpdateScript
participant FileSystem
User->>SelfUpdateScript: run_self_update
SelfUpdateScript->>FileSystem: compute TargetExe NewExe BackupExe TempExe
SelfUpdateScript->>FileSystem: copy NewExe to TempExe (same dir)
Note right of SelfUpdateScript: Stage new binary as TempExe
SelfUpdateScript->>FileSystem: move TargetExe to BackupExe
SelfUpdateScript->>FileSystem: move TempExe to TargetExe
alt swap_success
SelfUpdateScript->>FileSystem: remove BackupExe
SelfUpdateScript->>FileSystem: start TargetExe with --version
else swap_failure
SelfUpdateScript->>SelfUpdateScript: catch error
SelfUpdateScript->>FileSystem: check BackupExe exists
SelfUpdateScript->>FileSystem: move BackupExe to TargetExe
SelfUpdateScript->>FileSystem: check TempExe exists
SelfUpdateScript->>FileSystem: remove TempExe
SelfUpdateScript-->>User: propagate error
end
Flow diagram for updated portable binary swap and rollbackflowchart TD
A[Start self update] --> B[Compute paths for TargetExe NewExe BackupExe TempExe]
B --> C[Copy NewExe to TempExe in target directory]
C --> D[Try swap]
D --> E[Move TargetExe to BackupExe]
E --> F[Move TempExe to TargetExe]
F --> G{Swap succeeded}
G -->|Yes| H[Remove BackupExe]
H --> I[Start new TargetExe with --version]
I --> Z[End]
G -->|No exception| J[Log swap failed attempting rollback]
J --> K{BackupExe exists}
K -->|Yes| L[Move BackupExe back to TargetExe]
K -->|No| M[Skip restore original binary]
L --> N
M --> N[Continue rollback]
N --> O{TempExe exists}
O -->|Yes| P[Remove TempExe]
O -->|No| Q[Skip temp cleanup]
P --> R[Throw error]
Q --> R[Throw error]
R --> Z[End]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue | ||
| Move-Item -Path $TempExe -Destination $TargetExe -Force | ||
| Remove-Item -Path $BackupExe -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
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 👍 / 👎.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider wrapping the staging
Copy-Iteminto the sametry/catchflow (with afinally) so that failures during the temp copy also trigger consistent cleanup of$TempExeand don't leave partial temp files behind. - If there’s any chance of concurrent updates, using a unique temp filename (e.g., including a PID or GUID) instead of
$TargetExe.tmpwould avoid collisions between multiple updater processes operating in the same directory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the staging `Copy-Item` into the same `try`/`catch` flow (with a `finally`) so that failures during the temp copy also trigger consistent cleanup of `$TempExe` and don't leave partial temp files behind.
- If there’s any chance of concurrent updates, using a unique temp filename (e.g., including a PID or GUID) instead of `$TargetExe.tmp` would avoid collisions between multiple updater processes operating in the same directory.
## Individual Comments
### Comment 1
<location> `src/core/selfUpdate.ts:324-333` </location>
<code_context>
+Write-Host "Staging new binary..."
+$TempExe = "$TargetExe.tmp"
+Copy-Item -Path $NewExe -Destination $TempExe -Force
+
Write-Host "Swapping binaries..."
</code_context>
<issue_to_address>
**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.
```suggestion
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 {
```
</issue_to_address>
### Comment 2
<location> `src/core/selfUpdate.ts:333-342` </location>
<code_context>
+} catch {
+ Write-Host "Swap failed, attempting rollback..."
+ if (Test-Path $BackupExe) {
+ Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction SilentlyContinue
+ }
+ if (Test-Path $TempExe) {
</code_context>
<issue_to_address>
**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.
```suggestion
} 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
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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 { |
There was a problem hiding this comment.
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.
| 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 { |
| } 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 | ||
| } |
There was a problem hiding this comment.
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.
| } 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 | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR enhances the portable executable upgrade process by introducing a more robust file swap mechanism with staging, rollback capabilities, and cleanup. The changes address issue #22 by improving the reliability of self-updates for portable installations.
Key changes:
- Introduces a temporary staging file (.tmp) to prepare the new binary before the atomic swap operation
- Adds try-catch error handling around the swap operation with automatic rollback on failure
- Implements cleanup of backup files upon successful swap
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if (Test-Path $TempExe) { | ||
| Remove-Item -Path $TempExe -Force -ErrorAction SilentlyContinue | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| } | |
| if (Test-Path $NewExe) { | |
| Remove-Item -Path $NewExe -Force -ErrorAction SilentlyContinue | |
| } |
|
|
||
| Write-Host "Staging new binary..." | ||
| $TempExe = "$TargetExe.tmp" | ||
| Copy-Item -Path $NewExe -Destination $TempExe -Force |
There was a problem hiding this comment.
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.
| 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 | |
| } |
| 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 |
There was a problem hiding this comment.
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.
| 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." | |
| } |
| Move-Item -Path $BackupExe -Destination $TargetExe -Force -ErrorAction SilentlyContinue | ||
| } | ||
| if (Test-Path $TempExe) { | ||
| Remove-Item -Path $TempExe -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
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.
| 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 |
| try { | ||
| Move-Item -Path $TargetExe -Destination $BackupExe -Force -ErrorAction SilentlyContinue | ||
| Move-Item -Path $TempExe -Destination $TargetExe -Force | ||
| Remove-Item -Path $BackupExe -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
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.
1 participant
Closes #22
Summary:
Tests:
Rollback:
Summary by Sourcery
Improve the portable self-update swap process by staging the new binary via a temp file, performing an atomic replacement, and adding rollback behavior on failure.
Bug Fixes:
Enhancements: