From 63d8b5c28d12a1d6f5e60886f9c04b956760ffc4 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Tue, 4 Aug 2026 17:08:12 +0200 Subject: [PATCH] fix(installer): unbreak the update path and cache the download Every update failed at "Could not replace the existing files": the app relaunches the installer via ShellExecute without a working directory, so it inherited the app's CWD - which the Start Menu shortcut sets to \app. A process's current directory is locked by Windows, so the installer blocked its own `app` -> `app.bak` rename. Retries and reboots could not help. - installer moves its CWD to %TEMP% at startup, and both relaunch sites in the UI pass an explicit WorkingDirectory - cache the release zip in %TEMP%\ClaudeDo-download-cache and reuse it on a retry while its SHA-256 still matches, so a failed attempt no longer costs another full download; drop it after a successful install, delete a mismatching one, prune zips of other versions - roll back a half-done stash: a leftover app.bak was deleted as a stale stash on the next attempt, and that copy was the only one left - name the blocked path in the error message --- src/ClaudeDo.Installer/App.xaml.cs | 9 +- src/ClaudeDo.Installer/CLAUDE.md | 6 + .../Steps/DownloadAndExtractStep.cs | 102 ++++++++++---- .../ViewModels/IslandsShellViewModel.cs | 8 +- .../Modals/WorkerConnectionModalViewModel.cs | 9 +- .../DownloadAndExtractStepTests.cs | 125 +++++++++++++++++- 6 files changed, 224 insertions(+), 35 deletions(-) diff --git a/src/ClaudeDo.Installer/App.xaml.cs b/src/ClaudeDo.Installer/App.xaml.cs index a37d80f0..89207019 100644 --- a/src/ClaudeDo.Installer/App.xaml.cs +++ b/src/ClaudeDo.Installer/App.xaml.cs @@ -27,6 +27,13 @@ public partial class App : Application { base.OnStartup(e); + // The app relaunches us via ShellExecute without an explicit working directory, so we + // inherit its CWD — which the Start Menu shortcut sets to \app. A process's + // current directory is locked by the OS, so we would block DownloadAndExtractStep's + // Directory.Move of app\ against ourselves (fails on every attempt, reboot included). + // Step out of the install tree before anything else touches it. + try { Environment.CurrentDirectory = Path.GetTempPath(); } catch { /* best effort */ } + // --- Initialize localizer as early as possible so all windows can use {loc:Tr} --- var localesDir = Path.Combine(AppContext.BaseDirectory, "locales"); var localeStore = LocaleStore.Load(localesDir); @@ -124,7 +131,7 @@ public partial class App : Application // Steps — execution order matters for the FreshInstall pipeline (IEnumerable). // Double-registered as both IInstallStep and concrete type so the Update pipeline // can pull them out individually via GetRequiredService(). - sc.AddSingleton(); + sc.AddSingleton(sp => new DownloadAndExtractStep(sp.GetRequiredService())); sc.AddSingleton(sp => sp.GetRequiredService()); sc.AddSingleton(); sc.AddSingleton(); diff --git a/src/ClaudeDo.Installer/CLAUDE.md b/src/ClaudeDo.Installer/CLAUDE.md index 2b4181ed..8ad5776d 100644 --- a/src/ClaudeDo.Installer/CLAUDE.md +++ b/src/ClaudeDo.Installer/CLAUDE.md @@ -82,6 +82,12 @@ Non-fatal if `claude` CLI is missing or too old (prints the manual command). Ser No new service or scheduled task is created. Rationale: the worker must run in the user's interactive session so Claude CLI auth works. +**`DownloadAndExtractStep`** — fetches `checksums.txt` first and only touches the install dir after the zip verifies. The zip is cached in `%TEMP%\ClaudeDo-download-cache` (ctor takes an override for tests) and reused on a retry when its SHA-256 still matches, so a failed attempt doesn't cost another full download; it is dropped after a successful install, a bad download is deleted immediately, and zips of other versions are pruned. `app\`/`worker\` are stashed to `*.bak` before extraction and restored if extraction fails. + +### Gotcha: the installer must never run from inside the install dir + +`App.OnStartup` sets `Environment.CurrentDirectory` to `%TEMP%`, and the UI passes an explicit `WorkingDirectory` when it relaunches us. A process's current directory is locked by Windows: inheriting the app's CWD (`\app`, from the Start Menu shortcut's "start in") made the installer block its own `app` → `app.bak` rename, so every update failed with "Could not replace the existing files" — unaffected by retries or a reboot. Keep both guards. + ## `InstallContext` Defaults | Property | Default | diff --git a/src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs b/src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs index a2cef139..954b2682 100644 --- a/src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs +++ b/src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs @@ -9,10 +9,14 @@ namespace ClaudeDo.Installer.Steps; public sealed class DownloadAndExtractStep : IInstallStep { private readonly IReleaseClient _releases; + private readonly string _cacheDir; - public DownloadAndExtractStep(IReleaseClient releases) + public DownloadAndExtractStep(IReleaseClient releases, string? cacheDirectory = null) { _releases = releases; + // Downloads survive a failed attempt so a retry doesn't pull ~100 MB again. + // %TEMP% because Storage Sense ages the cache out on its own. + _cacheDir = cacheDirectory ?? Path.Combine(Path.GetTempPath(), "ClaudeDo-download-cache"); } public string Name => "Download and Extract"; @@ -38,38 +42,50 @@ public sealed class DownloadAndExtractStep : IInstallStep if (checksumAsset is null) return StepResult.Fail("checksums.txt not found in release metadata."); - var scratchDir = Path.Combine(Path.GetTempPath(), "ClaudeDo-install-" + Guid.NewGuid().ToString("N")); - Directory.CreateDirectory(scratchDir); + Directory.CreateDirectory(_cacheDir); + var zipPath = Path.Combine(_cacheDir, zipAsset.Name); + var checksumPath = Path.Combine(_cacheDir, "checksums.txt"); + PruneCacheExcept(zipAsset.Name); - try { - var zipPath = Path.Combine(scratchDir, zipAsset.Name); - var checksumPath = Path.Combine(scratchDir, "checksums.txt"); - - var totalMb = zipAsset.Size / (1024 * 1024); - progress.Report($"Downloading {zipAsset.Name} ({totalMb} MB)..."); - long lastReportedMb = -1; - await _releases.DownloadAsync(zipAsset.BrowserDownloadUrl, zipPath, - new Progress(b => - { - var mb = b / (1024 * 1024); - if (mb == lastReportedMb) return; - lastReportedMb = mb; - // Leading "\r" tells the UI to overwrite the previous line instead of appending. - progress.Report($"\r {mb} / {totalMb} MB downloaded"); - }), - ct); - progress.Report("Downloading checksums..."); await _releases.DownloadAsync(checksumAsset.BrowserDownloadUrl, checksumPath, new Progress(_ => { }), ct); - progress.Report("Verifying checksum..."); var map = ChecksumVerifier.ParseChecksumsFile(await File.ReadAllTextAsync(checksumPath, ct)); if (!map.TryGetValue(zipAsset.Name, out var expectedHash)) return StepResult.Fail($"No checksum entry for {zipAsset.Name} in checksums.txt."); - if (!ChecksumVerifier.Verify(zipPath, expectedHash)) - return StepResult.Fail("Checksum mismatch — the downloaded zip may be corrupt or tampered with."); + + // An earlier attempt may have failed after the download (locked files, bad + // extraction). Reuse that zip when it still verifies rather than re-downloading. + if (File.Exists(zipPath) && ChecksumVerifier.Verify(zipPath, expectedHash)) + { + progress.Report($"Reusing the already downloaded {zipAsset.Name}."); + } + else + { + var totalMb = zipAsset.Size / (1024 * 1024); + progress.Report($"Downloading {zipAsset.Name} ({totalMb} MB)..."); + long lastReportedMb = -1; + await _releases.DownloadAsync(zipAsset.BrowserDownloadUrl, zipPath, + new Progress(b => + { + var mb = b / (1024 * 1024); + if (mb == lastReportedMb) return; + lastReportedMb = mb; + // Leading "\r" tells the UI to overwrite the previous line instead of appending. + progress.Report($"\r {mb} / {totalMb} MB downloaded"); + }), + ct); + + progress.Report("Verifying checksum..."); + if (!ChecksumVerifier.Verify(zipPath, expectedHash)) + { + // Never keep a bad download around — it would be re-verified forever. + TryDelete(zipPath); + return StepResult.Fail("Checksum mismatch — the downloaded zip may be corrupt or tampered with."); + } + } // Only after verification do we touch the install directory. progress.Report("Stashing previous app/worker binaries..."); @@ -78,20 +94,31 @@ public sealed class DownloadAndExtractStep : IInstallStep var appBak = appDest + ".bak"; var workerBak = workerDest + ".bak"; + var stashedApp = false; + var failedPath = ctx.InstallDirectory; try { + failedPath = appBak; if (Directory.Exists(appBak)) DeleteWithRetry(appBak); + failedPath = workerBak; if (Directory.Exists(workerBak)) DeleteWithRetry(workerBak); - if (Directory.Exists(appDest)) MoveWithRetry(appDest, appBak); + failedPath = appDest; + if (Directory.Exists(appDest)) { MoveWithRetry(appDest, appBak); stashedApp = true; } + failedPath = workerDest; if (Directory.Exists(workerDest)) MoveWithRetry(workerDest, workerBak); } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { + // Undo a half-done stash: a leftover app.bak would be deleted as a stale + // stash on the next attempt — that copy is the only one we still have. + if (stashedApp && !Directory.Exists(appDest) && Directory.Exists(appBak)) + try { MoveWithRetry(appBak, appDest); } catch { /* best effort */ } + // A just-stopped app/worker (or an Explorer/terminal window sitting in // the install dir) still held a handle. Surface an actionable message // instead of the raw "process cannot access the file" error. return StepResult.Fail( - "Could not replace the existing app/worker files — they are still in use. " + + $"Could not replace '{failedPath}' — it is still in use. " + "Make sure ClaudeDo is fully closed (app and worker) and no Explorer or " + $"terminal window is open inside the install folder, then run the update again. Details: {ex.Message}"); } @@ -117,13 +144,32 @@ public sealed class DownloadAndExtractStep : IInstallStep if (Directory.Exists(appBak)) DeleteWithRetry(appBak); if (Directory.Exists(workerBak)) DeleteWithRetry(workerBak); + // Installed — the cached zip has served its purpose. + TryDelete(zipPath); + TryDelete(checksumPath); + ctx.InstalledVersion = release.TagName.TrimStart('v', 'V'); return StepResult.Ok(); } - finally + } + + // Zips from earlier attempts on other versions would pile up otherwise. + private void PruneCacheExcept(string keepFileName) + { + try { - try { Directory.Delete(scratchDir, recursive: true); } catch { /* best effort */ } + foreach (var file in Directory.EnumerateFiles(_cacheDir, "*.zip")) + { + if (!string.Equals(Path.GetFileName(file), keepFileName, StringComparison.OrdinalIgnoreCase)) + TryDelete(file); + } } + catch { /* best effort */ } + } + + private static void TryDelete(string file) + { + try { File.Delete(file); } catch { /* best effort */ } } private static void MoveWithRetry(string source, string dest) diff --git a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs index 43ad8518..aedb31ed 100644 --- a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs @@ -496,7 +496,13 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable try { - System.Diagnostics.Process.Start(new System.Diagnostics.ProcessStartInfo(path) { UseShellExecute = true }); + // WorkingDirectory must NOT stay empty: the child would inherit ours (\app) + // and its locked current directory blocks the installer's own app\ rename. + System.Diagnostics.Process.Start(new System.Diagnostics.ProcessStartInfo(path) + { + UseShellExecute = true, + WorkingDirectory = System.IO.Path.GetTempPath(), + }); Environment.Exit(0); } catch diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/WorkerConnectionModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/WorkerConnectionModalViewModel.cs index dcc9b5a6..48e4b5f6 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/WorkerConnectionModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/WorkerConnectionModalViewModel.cs @@ -1,5 +1,6 @@ using System; using System.Diagnostics; +using System.IO; using ClaudeDo.Ui.Services; using CommunityToolkit.Mvvm.Input; @@ -37,7 +38,13 @@ public sealed partial class WorkerConnectionModalViewModel : ViewModelBase if (path is null) return; try { - Process.Start(new ProcessStartInfo(path) { UseShellExecute = true }); + // See IslandsShellViewModel.UpdateNow: an inherited CWD inside the install dir + // makes the installer block its own app\ rename. + Process.Start(new ProcessStartInfo(path) + { + UseShellExecute = true, + WorkingDirectory = Path.GetTempPath(), + }); Environment.Exit(0); } catch { /* nothing useful to show */ } diff --git a/tests/ClaudeDo.Installer.Tests/DownloadAndExtractStepTests.cs b/tests/ClaudeDo.Installer.Tests/DownloadAndExtractStepTests.cs index cbba0df1..8ef623ee 100644 --- a/tests/ClaudeDo.Installer.Tests/DownloadAndExtractStepTests.cs +++ b/tests/ClaudeDo.Installer.Tests/DownloadAndExtractStepTests.cs @@ -10,6 +10,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable { private readonly string _tempDir; private readonly string _installDir; + private readonly string _cacheDir; public DownloadAndExtractStepTests() { @@ -17,6 +18,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable Directory.CreateDirectory(_tempDir); _installDir = Path.Combine(_tempDir, "install"); Directory.CreateDirectory(_installDir); + _cacheDir = Path.Combine(_tempDir, "cache"); } public void Dispose() @@ -28,6 +30,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable { private readonly Dictionary _urlToSourceFile; public GiteaRelease? Release { get; set; } + public List DownloadedUrls { get; } = new(); public FileCopyReleaseClient(Dictionary urlToSourceFile) => _urlToSourceFile = urlToSourceFile; @@ -36,12 +39,45 @@ public sealed class DownloadAndExtractStepTests : IDisposable public Task DownloadAsync(string url, string destPath, IProgress progress, CancellationToken ct) { + DownloadedUrls.Add(url); File.Copy(_urlToSourceFile[url], destPath, overwrite: true); progress.Report(new FileInfo(destPath).Length); return Task.CompletedTask; } } + // Builds a valid release zip + checksums.txt and a client that serves both. + private (FileCopyReleaseClient Client, string ZipName) SetUpRelease() + { + var zipPath = Path.Combine(_tempDir, "release.zip"); + using (var fs = File.Create(zipPath)) + using (var zip = new ZipArchive(fs, ZipArchiveMode.Create)) + { + var a = zip.CreateEntry("app/a.txt"); + using (var w = new StreamWriter(a.Open())) w.Write("hello-app"); + var b = zip.CreateEntry("worker/b.txt"); + using (var w = new StreamWriter(b.Open())) w.Write("hello-worker"); + } + + const string zipName = "ClaudeDo-0.1.0-win-x64.zip"; + var checksumsPath = Path.Combine(_tempDir, "checksums.txt"); + File.WriteAllText(checksumsPath, $"{ChecksumVerifier.ComputeSha256(zipPath)} {zipName}\n"); + + var release = new GiteaRelease("v0.1.0", "v0.1.0", new[] + { + new ReleaseAsset(zipName, "fake://zip", new FileInfo(zipPath).Length), + new ReleaseAsset("checksums.txt", "fake://checksums", new FileInfo(checksumsPath).Length), + }); + + var client = new FileCopyReleaseClient(new() + { + ["fake://zip"] = zipPath, + ["fake://checksums"] = checksumsPath, + }) { Release = release }; + + return (client, zipName); + } + [Fact] public async Task Extracts_Zip_Into_InstallDir_App_And_Worker() { @@ -71,7 +107,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable ["fake://checksums"] = checksumsPath, }) { Release = release }; - var step = new DownloadAndExtractStep(client); + var step = new DownloadAndExtractStep(client, _cacheDir); var ctx = new InstallContext { InstallDirectory = _installDir }; var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); @@ -110,7 +146,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable ["fake://checksums"] = checksumsPath, }) { Release = release }; - var step = new DownloadAndExtractStep(client); + var step = new DownloadAndExtractStep(client, _cacheDir); var ctx = new InstallContext { InstallDirectory = _installDir }; var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); @@ -126,7 +162,7 @@ public sealed class DownloadAndExtractStepTests : IDisposable { var release = new GiteaRelease("v0.1.0", "v0.1.0", Array.Empty()); var client = new FileCopyReleaseClient(new()) { Release = release }; - var step = new DownloadAndExtractStep(client); + var step = new DownloadAndExtractStep(client, _cacheDir); var ctx = new InstallContext { InstallDirectory = _installDir }; var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); @@ -135,11 +171,92 @@ public sealed class DownloadAndExtractStepTests : IDisposable Assert.Contains("not found", result.ErrorMessage!, StringComparison.OrdinalIgnoreCase); } + [Fact] + public async Task Reuses_Cached_Zip_Instead_Of_Downloading_It_Again() + { + var (client, zipName) = SetUpRelease(); + Directory.CreateDirectory(_cacheDir); + // A previous run left a fully downloaded, still-valid zip behind. + File.Copy(Path.Combine(_tempDir, "release.zip"), Path.Combine(_cacheDir, zipName)); + + var step = new DownloadAndExtractStep(client, _cacheDir); + var ctx = new InstallContext { InstallDirectory = _installDir }; + + var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + + Assert.True(result.Success, result.ErrorMessage); + Assert.DoesNotContain("fake://zip", client.DownloadedUrls); + Assert.Equal("hello-app", File.ReadAllText(Path.Combine(_installDir, "app", "a.txt"))); + } + + [Fact] + public async Task Redownloads_When_Cached_Zip_Is_Corrupt() + { + var (client, zipName) = SetUpRelease(); + Directory.CreateDirectory(_cacheDir); + // A cancelled/aborted download left a truncated file behind. + File.WriteAllText(Path.Combine(_cacheDir, zipName), "not-a-zip"); + + var step = new DownloadAndExtractStep(client, _cacheDir); + var ctx = new InstallContext { InstallDirectory = _installDir }; + + var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + + Assert.True(result.Success, result.ErrorMessage); + Assert.Contains("fake://zip", client.DownloadedUrls); + Assert.Equal("hello-app", File.ReadAllText(Path.Combine(_installDir, "app", "a.txt"))); + } + + [Fact] + public async Task Keeps_Zip_Cached_When_The_Install_Fails_And_Drops_It_On_Success() + { + var (client, zipName) = SetUpRelease(); + var cachedZip = Path.Combine(_cacheDir, zipName); + + // Make extraction fail: a *file* named "app" blocks the app/ entry's directory. + var blocker = Path.Combine(_installDir, "app"); + File.WriteAllText(blocker, "blocked"); + + var step = new DownloadAndExtractStep(client, _cacheDir); + var ctx = new InstallContext { InstallDirectory = _installDir }; + + var failed = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + Assert.False(failed.Success); + Assert.True(File.Exists(cachedZip), "a failed install must keep the download cached"); + + // Retry after clearing the blocker: no second download, and the cache is dropped. + File.Delete(blocker); + client.DownloadedUrls.Clear(); + + var ok = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + + Assert.True(ok.Success, ok.ErrorMessage); + Assert.DoesNotContain("fake://zip", client.DownloadedUrls); + Assert.False(File.Exists(cachedZip), "a successful install must not leave the zip behind"); + } + + [Fact] + public async Task Prunes_Cached_Zips_From_Other_Versions() + { + var (client, _) = SetUpRelease(); + Directory.CreateDirectory(_cacheDir); + var stale = Path.Combine(_cacheDir, "ClaudeDo-0.0.9-win-x64.zip"); + File.WriteAllText(stale, "old release"); + + var step = new DownloadAndExtractStep(client, _cacheDir); + var ctx = new InstallContext { InstallDirectory = _installDir }; + + var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + + Assert.True(result.Success, result.ErrorMessage); + Assert.False(File.Exists(stale)); + } + [Fact] public async Task Fails_When_ReleaseClient_Returns_Null() { var client = new FileCopyReleaseClient(new()) { Release = null }; - var step = new DownloadAndExtractStep(client); + var step = new DownloadAndExtractStep(client, _cacheDir); var ctx = new InstallContext { InstallDirectory = _installDir }; var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None);