fix(update): stop crashing on Windows after a successful update - #1924
Merged
Conversation
The post-update cleanup tried to delete the backup of the currently running CLI binary. On Windows, DeleteFile on a running image fails with UnauthorizedAccessException, turning a successful update into a fatal crash. Skip deleting the running process's own backup on Windows; the install step removes stale backups before renaming on the next update. Treat any other backup-delete failure as a warning, and harden the install step's backup swap against transient lock failures with a clear error instead of an unhandled exception. Closes #1923
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+545
to
+548
| catch (Exception ex) | ||
| { | ||
| Console.Error.WriteLine($"warn: could not remove backup {backupPath}: {ex.Message}"); | ||
| } |
| [Fact] | ||
| public void CleanupBackupFile_DoesNotDelete_RunningImageBackup_OnWindows() | ||
| { | ||
| var backupPath = Path.Combine(_dir.Path, "netclaw.exe.backup"); |
A failed swap could leave the install directory without a binary: the old executable was renamed to .backup, then the new one failed to move into place. On Windows that bricks the CLI until the user manually restores the .backup. SwapBinaryIntoPlace now restores the previous binary to the target path when the new binary fails to move, and reports the rollback outcome in the error message. The install step also tells the user the daemon is stopped and how to recover. Adds tests for the swap success path, rollback on move failure, and stale-backup delete failure leaving the target intact.
Comment on lines
+548
to
+553
| catch (Exception rollbackEx) | ||
| { | ||
| // Best-effort rollback; the original swap failure is | ||
| // rethrown below and reported to the user. | ||
| Console.Error.WriteLine($"warn: failed to restore {targetPath} from {backupPath}: {rollbackEx.Message}"); | ||
| } |
| // from; on Windows DeleteFile fails with UnauthorizedAccessException. | ||
| // NTFS path comparison is case-insensitive, so pin that here — a | ||
| // regression to Ordinal would leave the backup deleted. | ||
| var runningBackupPath = Path.Combine(_dir.Path, "NETCLAW.EXE.BACKUP"); |
| [Fact] | ||
| public void SwapBinaryIntoPlace_ReplacesTarget_AndBacksUpOldBinary() | ||
| { | ||
| var sourcePath = Path.Combine(_dir.Path, "new.exe"); |
| public void SwapBinaryIntoPlace_ReplacesTarget_AndBacksUpOldBinary() | ||
| { | ||
| var sourcePath = Path.Combine(_dir.Path, "new.exe"); | ||
| var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); |
| [Fact] | ||
| public void SwapBinaryIntoPlace_RestoresOldBinary_WhenNewBinaryMoveFails() | ||
| { | ||
| var sourcePath = Path.Combine(_dir.Path, "new.exe"); |
| public void SwapBinaryIntoPlace_RestoresOldBinary_WhenNewBinaryMoveFails() | ||
| { | ||
| var sourcePath = Path.Combine(_dir.Path, "new.exe"); | ||
| var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); |
| if (OperatingSystem.IsWindows() || Environment.UserName == "root") | ||
| return; // permission simulation below is Unix-only and ineffective for root | ||
|
|
||
| var sourcePath = Path.Combine(_dir.Path, "new.exe"); |
The chmod-based file-lock simulation tests used an inline `if (OperatingSystem.IsWindows() || root) return;` early-exit, which reports as Passed on unsupported platforms. Switch to the repo standard used by the shell-approval suite: a static SkipUnless hook (`CanSimulateFileLock`) plus a local SlopwatchSuppressAttribute. CA1416 still requires a recognized platform guard for the File.GetUnixFileMode/SetUnixFileMode calls (xUnit attributes are invisible to the analyzer), so the Windows early-return stays as the analyzer guard — matching SecretsFileWriterTests, the repo's other UnixFileMode test precedent.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1923
Problem
netclaw updateon Windows swaps the running CLI binary, restarts the daemon, then crashes with an unhandledUnauthorizedAccessExceptionwhile deletingnetclaw.exe.backupin the post-update cleanup loop.Root cause: the install step renames the currently running
netclaw.exetonetclaw.exe.backup(rename of a running image is allowed on Windows), then cleanup callsFile.Deleteon it. Windows refuses to delete the image a process is still executing from. The update had already succeeded — the crash was cosmetic cleanup turning success into a fatal error.Fix
src/Netclaw.Cli/Update/UpdateCommand.csEnvironment.ProcessPath + ".backup"), not component-name matching, so it also covers daemon self-update. The leftover self-heals: the install step deletes stale backups before renaming on the next update.CleanupBackupFile) — a leftover backup must never turn a successful update into a fatal error..backup): fail loudly with a clear message andreturn 1instead of crashing mid-swap.On Linux/macOS the rename/delete of a running image is safe (POSIX unlink semantics), so behavior there is unchanged — backups are still created and cleaned up.
Tests
Added 4 unit tests in
UpdateCommandTests.cscovering:All 42
UpdateCommandTestspass; build clean; slopwatch no new violations; file headers verified.Notes