From e69f8e5942fd2992077fa123fbf06187dc3f2ddb Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 13 Aug 2026 15:26:59 +0000 Subject: [PATCH 1/3] fix(update): stop crashing on Windows after a successful update (#1923) 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 --- .../Cli/UpdateCommandTests.cs | 69 +++++++++++++++++ src/Netclaw.Cli/Update/UpdateCommand.cs | 75 ++++++++++++++++--- 2 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs index a47a4ea83..934a6cefb 100644 --- a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs @@ -303,6 +303,75 @@ public async Task RunAsync_RejectsUnknownChannel() return doc.RootElement.GetProperty("Daemon").GetProperty("UpdateChannel").GetString(); } + [Fact] + public void CleanupBackupFile_DoesNotDelete_RunningImageBackup_OnWindows() + { + var backupPath = Path.Combine(_dir.Path, "netclaw.exe.backup"); + File.WriteAllText(backupPath, "old image"); + + // The running process's backup is the very image this process executes + // from; on Windows DeleteFile fails with UnauthorizedAccessException. + UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath: backupPath, isWindows: true); + + Assert.True(File.Exists(backupPath)); + } + + [Fact] + public void CleanupBackupFile_Deletes_OtherComponentBackup_OnWindows() + { + var backupPath = Path.Combine(_dir.Path, "netclawd.exe.backup"); + File.WriteAllText(backupPath, "old image"); + var runningBackupPath = Path.Combine(_dir.Path, "netclaw.exe") + ".backup"; + + UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath, isWindows: true); + + Assert.False(File.Exists(backupPath)); + } + + [Fact] + public void CleanupBackupFile_Deletes_Backup_OnNonWindows() + { + var backupPath = Path.Combine(_dir.Path, "netclaw.backup"); + File.WriteAllText(backupPath, "old image"); + + // POSIX allows unlinking a running image, so even the running + // process's own backup is removed. + UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath: backupPath, isWindows: false); + + Assert.False(File.Exists(backupPath)); + } + + [Fact] + public void CleanupBackupFile_DoesNotThrow_WhenDeleteFails() + { + if (OperatingSystem.IsWindows() || Environment.UserName == "root") + return; // permission simulation below is Unix-only and ineffective for root + + var backupPath = Path.Combine(_dir.Path, "netclaw.backup"); + File.WriteAllText(backupPath, "old image"); + var dir = Path.GetDirectoryName(backupPath)!; + var originalMode = File.GetUnixFileMode(dir); + + try + { + // Remove write permission on the directory so unlink fails with + // UnauthorizedAccessException — the same failure class as deleting + // a running image on Windows. + File.SetUnixFileMode(dir, UnixFileMode.UserRead | UnixFileMode.UserExecute + | UnixFileMode.GroupRead | UnixFileMode.GroupExecute + | UnixFileMode.OtherRead | UnixFileMode.OtherExecute); + + UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath: null, isWindows: false); + + // Warned, not crashed; the leftover self-heals on the next update. + Assert.True(File.Exists(backupPath)); + } + finally + { + File.SetUnixFileMode(dir, originalMode); + } + } + [Theory] [MemberData(nameof(StartupUpdateSkippedCases))] public void ShouldRunStartupUpdateCheck_ReturnsFalse_ForInteractiveOrSelfUpdateFlows(string[] args) diff --git a/src/Netclaw.Cli/Update/UpdateCommand.cs b/src/Netclaw.Cli/Update/UpdateCommand.cs index efef46e32..84dbc58bf 100644 --- a/src/Netclaw.Cli/Update/UpdateCommand.cs +++ b/src/Netclaw.Cli/Update/UpdateCommand.cs @@ -267,15 +267,26 @@ private static async Task PerformUpdateAsync( var targetPath = Path.Combine(installDir, binaryName); var backupPath = targetPath + ".backup"; - // Backup existing binary - if (File.Exists(targetPath)) + // Backup existing binary, then move the new one into place. + // A stale backup may be transiently locked (AV scan); fail + // loudly instead of crashing the process mid-swap. + try { - if (File.Exists(backupPath)) - File.Delete(backupPath); - File.Move(targetPath, backupPath); - } + if (File.Exists(targetPath)) + { + if (File.Exists(backupPath)) + File.Delete(backupPath); + File.Move(targetPath, backupPath); + } - File.Move(sourcePath, targetPath); + File.Move(sourcePath, targetPath); + } + catch (Exception ex) + { + Console.WriteLine($"\n Failed to replace {binaryName}: {ex.Message}"); + Console.WriteLine(" If the install directory is left without a binary, re-run 'netclaw update' to complete the swap."); + return 1; + } // Set executable permission on Unix if (!OperatingSystem.IsWindows()) @@ -299,15 +310,23 @@ private static async Task PerformUpdateAsync( Console.WriteLine(" done."); } - // Clean up backup files + // Clean up backup files. The backup of the currently running CLI + // binary is the image this process still executes from; on Windows + // DeleteFile on a running image fails with + // UnauthorizedAccessException. Leave it — the install step deletes + // stale backups before moving the new binary on the next update. + // A leftover backup must never turn a successful update into a + // fatal error, so any other delete failure only warns. + var runningBackupPath = Environment.ProcessPath is { } processPath + ? processPath + ".backup" + : null; foreach (var (component, _) in extractedPaths) { var binaryName = OperatingSystem.IsWindows() ? $"{component}.exe" : component; var backupPath = Path.Combine(installDir, binaryName + ".backup"); - if (File.Exists(backupPath)) - File.Delete(backupPath); + CleanupBackupFile(backupPath, runningBackupPath, OperatingSystem.IsWindows()); } Console.WriteLine($"\nUpdated to v{result.LatestVersion}."); @@ -493,6 +512,42 @@ private static async Task ExtractTarGzAsync(string archivePath, string extractDi return null; } + /// + /// Deletes a leftover .backup file after a successful update. + /// On Windows the backup of the currently running image cannot be + /// deleted — the process still executes from that file, so DeleteFile + /// fails with . Such backups are + /// removed by the install step on the next update, before it renames the + /// new binary. Any other delete failure only warns; a leftover backup must + /// never turn a successful update into a fatal error. + /// + /// The backup file to delete. + /// + /// The backup path of the currently running process + /// (Environment.ProcessPath + ".backup"), or null if unknown. + /// + /// True when running on Windows. + internal static void CleanupBackupFile(string backupPath, string? runningBackupPath, bool isWindows) + { + if (isWindows + && runningBackupPath is not null + && string.Equals(backupPath, runningBackupPath, StringComparison.OrdinalIgnoreCase)) + { + // Running image — cannot be deleted on Windows. + return; + } + + try + { + if (File.Exists(backupPath)) + File.Delete(backupPath); + } + catch (Exception ex) + { + Console.Error.WriteLine($"warn: could not remove backup {backupPath}: {ex.Message}"); + } + } + private static string GetInstallDirectory() { // Use the directory containing the current CLI binary From 86fbaa536beafa89a6b87de9337e1576ca9c71fb Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 13 Aug 2026 20:22:17 +0000 Subject: [PATCH 2/3] fix(update): roll back the swap on failure so installs never brick 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. --- .../Cli/UpdateCommandTests.cs | 75 ++++++++++++++++++- src/Netclaw.Cli/Update/UpdateCommand.cs | 68 ++++++++++++++--- 2 files changed, 130 insertions(+), 13 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs index 934a6cefb..06f5f4961 100644 --- a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs @@ -311,11 +311,84 @@ public void CleanupBackupFile_DoesNotDelete_RunningImageBackup_OnWindows() // The running process's backup is the very image this process executes // from; on Windows DeleteFile fails with UnauthorizedAccessException. - UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath: backupPath, isWindows: true); + // 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"); + UpdateCommand.CleanupBackupFile(backupPath, runningBackupPath, isWindows: true); Assert.True(File.Exists(backupPath)); } + [Fact] + public void SwapBinaryIntoPlace_ReplacesTarget_AndBacksUpOldBinary() + { + var sourcePath = Path.Combine(_dir.Path, "new.exe"); + var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); + var backupPath = targetPath + ".backup"; + File.WriteAllText(sourcePath, "new image"); + File.WriteAllText(targetPath, "old image"); + + UpdateCommand.SwapBinaryIntoPlace(sourcePath, targetPath, backupPath); + + Assert.Equal("new image", File.ReadAllText(targetPath)); + Assert.Equal("old image", File.ReadAllText(backupPath)); + } + + [Fact] + public void SwapBinaryIntoPlace_RestoresOldBinary_WhenNewBinaryMoveFails() + { + var sourcePath = Path.Combine(_dir.Path, "new.exe"); + var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); + var backupPath = targetPath + ".backup"; + File.WriteAllText(sourcePath, "new image"); + File.WriteAllText(targetPath, "old image"); + + // Make the final move fail after the old binary was backed up: the + // install directory must never be left without an executable. + File.Delete(sourcePath); + + Assert.ThrowsAny(() => UpdateCommand.SwapBinaryIntoPlace(sourcePath, targetPath, backupPath)); + // The old binary is rolled back into place; the backup is consumed by + // the restore, so the install directory is left with a working binary. + Assert.Equal("old image", File.ReadAllText(targetPath)); + Assert.False(File.Exists(backupPath)); + } + + [Fact] + public void SwapBinaryIntoPlace_LeavesTargetIntact_WhenStaleBackupDeleteFails() + { + 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"); + var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); + var backupPath = targetPath + ".backup"; + File.WriteAllText(sourcePath, "new image"); + File.WriteAllText(targetPath, "old image"); + File.WriteAllText(backupPath, "stale image"); + var dir = Path.GetDirectoryName(targetPath)!; + var originalMode = File.GetUnixFileMode(dir); + + try + { + // Remove write permission on the directory so the stale-backup + // delete fails with UnauthorizedAccessException — the same failure + // class as an AV-locked file on Windows. The target must be left + // untouched (no half-swap). + File.SetUnixFileMode(dir, UnixFileMode.UserRead | UnixFileMode.UserExecute + | UnixFileMode.GroupRead | UnixFileMode.GroupExecute + | UnixFileMode.OtherRead | UnixFileMode.OtherExecute); + + Assert.ThrowsAny(() => UpdateCommand.SwapBinaryIntoPlace(sourcePath, targetPath, backupPath)); + Assert.Equal("old image", File.ReadAllText(targetPath)); + Assert.Equal("stale image", File.ReadAllText(backupPath)); + } + finally + { + File.SetUnixFileMode(dir, originalMode); + } + } + [Fact] public void CleanupBackupFile_Deletes_OtherComponentBackup_OnWindows() { diff --git a/src/Netclaw.Cli/Update/UpdateCommand.cs b/src/Netclaw.Cli/Update/UpdateCommand.cs index 84dbc58bf..d3a75e4d4 100644 --- a/src/Netclaw.Cli/Update/UpdateCommand.cs +++ b/src/Netclaw.Cli/Update/UpdateCommand.cs @@ -267,24 +267,25 @@ private static async Task PerformUpdateAsync( var targetPath = Path.Combine(installDir, binaryName); var backupPath = targetPath + ".backup"; - // Backup existing binary, then move the new one into place. - // A stale backup may be transiently locked (AV scan); fail - // loudly instead of crashing the process mid-swap. try { - if (File.Exists(targetPath)) - { - if (File.Exists(backupPath)) - File.Delete(backupPath); - File.Move(targetPath, backupPath); - } - - File.Move(sourcePath, targetPath); + // Swap with automatic rollback: a failed swap restores the + // previous binary so the install directory is never left + // without an executable (which would brick the CLI). + SwapBinaryIntoPlace(sourcePath, targetPath, backupPath); } catch (Exception ex) { + var targetRestored = File.Exists(targetPath); Console.WriteLine($"\n Failed to replace {binaryName}: {ex.Message}"); - Console.WriteLine(" If the install directory is left without a binary, re-run 'netclaw update' to complete the swap."); + if (targetRestored) + { + Console.WriteLine(" The previous binary was restored. The daemon is stopped; start it with 'netclaw daemon start'."); + } + else + { + Console.WriteLine($" The install directory is missing {binaryName}. Restore it from {binaryName}.backup, then start the daemon with 'netclaw daemon start'."); + } return 1; } @@ -512,6 +513,49 @@ private static async Task ExtractTarGzAsync(string archivePath, string extractDi return null; } + /// + /// Replaces with + /// , preserving the previous binary at + /// . On failure the previous binary is + /// restored to so a failed swap never + /// leaves the install directory without an executable. + /// + /// The new binary to install. + /// The installed binary to replace. + /// Where the previous binary is preserved. + internal static void SwapBinaryIntoPlace(string sourcePath, string targetPath, string backupPath) + { + var movedOldToBackup = false; + try + { + if (File.Exists(targetPath)) + { + if (File.Exists(backupPath)) + File.Delete(backupPath); + File.Move(targetPath, backupPath); + movedOldToBackup = true; + } + + File.Move(sourcePath, targetPath); + } + catch + { + // Roll the previous binary back so a failed swap leaves a + // working binary in place instead of a missing executable. + if (movedOldToBackup && !File.Exists(targetPath) && File.Exists(backupPath)) + { + try { File.Move(backupPath, targetPath); } + 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}"); + } + } + throw; + } + } + /// /// Deletes a leftover .backup file after a successful update. /// On Windows the backup of the currently running image cannot be From 2fbe1139354d827f4327577baa5fdd095f2808ce Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 13 Aug 2026 20:48:19 +0000 Subject: [PATCH 3/3] test(update): use repo-standard SkipUnless for POSIX-only lock tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Cli/UpdateCommandTests.cs | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs index 06f5f4961..14395edf9 100644 --- a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs @@ -20,6 +20,15 @@ namespace Netclaw.Cli.Tests.Cli; [Collection("Update verification")] public sealed class UpdateCommandTests : IDisposable { + /// + /// xunit.v3 SkipUnless hook for tests that simulate Windows + /// file-lock failures by revoking directory write permission. The + /// simulation is POSIX-only (File.SetUnixFileMode) and is + /// ineffective for root, which bypasses directory permission bits. + /// + public static bool CanSimulateFileLock => + !OperatingSystem.IsWindows() && Environment.UserName != "root"; + private readonly DisposableTempDir _dir = new(); private readonly NetclawPaths _paths; private readonly Key _testSigningKey; @@ -354,11 +363,12 @@ public void SwapBinaryIntoPlace_RestoresOldBinary_WhenNewBinaryMoveFails() Assert.False(File.Exists(backupPath)); } - [Fact] + [Fact(SkipUnless = nameof(CanSimulateFileLock), Skip = "POSIX-only permission simulation (ineffective on Windows or as root)")] + [SlopwatchSuppress("SW001", "Simulates Windows file locks via POSIX directory permissions, which cannot run on Windows or as root.")] public void SwapBinaryIntoPlace_LeavesTargetIntact_WhenStaleBackupDeleteFails() { - if (OperatingSystem.IsWindows() || Environment.UserName == "root") - return; // permission simulation below is Unix-only and ineffective for root + if (OperatingSystem.IsWindows()) + return; // SkipUnless gates the skip; this guard satisfies CA1416 for Unix-only APIs var sourcePath = Path.Combine(_dir.Path, "new.exe"); var targetPath = Path.Combine(_dir.Path, "netclaw.exe"); @@ -414,11 +424,12 @@ public void CleanupBackupFile_Deletes_Backup_OnNonWindows() Assert.False(File.Exists(backupPath)); } - [Fact] + [Fact(SkipUnless = nameof(CanSimulateFileLock), Skip = "POSIX-only permission simulation (ineffective on Windows or as root)")] + [SlopwatchSuppress("SW001", "Simulates Windows file locks via POSIX directory permissions, which cannot run on Windows or as root.")] public void CleanupBackupFile_DoesNotThrow_WhenDeleteFails() { - if (OperatingSystem.IsWindows() || Environment.UserName == "root") - return; // permission simulation below is Unix-only and ineffective for root + if (OperatingSystem.IsWindows()) + return; // SkipUnless gates the skip; this guard satisfies CA1416 for Unix-only APIs var backupPath = Path.Combine(_dir.Path, "netclaw.backup"); File.WriteAllText(backupPath, "old image"); @@ -613,5 +624,15 @@ public Task RunAsync(string command, string arguments) : _results.Dequeue()); } } +} + +/// +/// Supplies source-level Slopwatch suppressions without a runtime package dependency. +/// +[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] +internal sealed class SlopwatchSuppressAttribute(string ruleId, string reason) : Attribute +{ + public string RuleId { get; } = ruleId; + public string Reason { get; } = reason; }