From a66c803dfefd288e679a4aaa712c294e1521e0aa Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 25 May 2026 05:35:05 +0000 Subject: [PATCH] v0.1.6: robust .old/ + staging cleanup under OneDrive / AV / Search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The swap moves the previous install's contents into .old/ and the next startup's CleanupOldInstall removes that tree. On Windows with OneDrive syncing the install directory, OneDrive grabs sync handles on the freshly-moved files within seconds; the recursive Directory.Delete walks happily through most of them but races a handle on the last few, throws IOException("being used by another process"), and the existing catch-all swallows it. Result: .old/ persists across startups, usually near-empty save for whatever OneDrive was still touching. Antivirus scanners, Windows Search, indexers, and backup agents create the same race. Files extracted from archives with the ReadOnly attribute hit a parallel UnauthorizedAccessException for similar timing reasons. Fix: new internal UpdateInstaller.DeleteDirectoryRobustly(path) helper that clears the ReadOnly attribute on every descendant file and retries the recursive delete at 200/400/800 ms on IOException / UnauthorizedAccessException — ~1.4 s total budget tuned to OneDrive's typical handle-release latency. Applied to all four recursive-delete sites: CleanupOldInstall, SwapAsync's .old/ reset, ResetStaging, TryDeleteDirectory. Exception-propagation contracts unchanged (CleanupOldInstall still swallows; the other three still throw via UpdateException). OneDrive detection deliberately avoided — fragile (registry queries, reparse-point sniffing) and the retry generalises to every other class of transient handle holder. Cost when no contention exists: one no-op attribute walk over a tree we're about to delete. Tests inject deleter + sleeper seams so the retry-with-backoff logic can be exercised deterministically without simulating real Windows locks. 172 → 177 passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 15 +++ ...Iteration.SpectreConsole.SelfUpdate.csproj | 2 +- .../Pipeline/UpdateInstaller.cs | 96 ++++++++++++--- .../Pipeline/UpdateInstallerTests.cs | 116 ++++++++++++++++++ 4 files changed, 214 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 005d540..dbf28a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 --- +## [0.1.6] — 2026-05-25 + +### Fixed + +- **`.old/` cleanup leaves a few stragglers on Windows when OneDrive (or antivirus / Windows Search) is syncing the install directory.** The swap moves the previous install's files into `.old/`, OneDrive picks them up for sync within seconds, and the next-startup `CleanupOldInstall` recursive delete races OneDrive's open handles — most files delete fine, the ones OneDrive is still touching throw `IOException("being used by another process")`, the catch-all swallows it, and the user sees `.old/` persist (often near-empty). Same race affects `SwapAsync`'s `.old/` reset and the staging `ResetStaging`. Read-only files extracted from archives hit a parallel `UnauthorizedAccessException` for similar reasons. +- Fix: new internal `UpdateInstaller.DeleteDirectoryRobustly(path)` helper that (1) clears the `ReadOnly` attribute on every descendant file before each attempt and (2) retries the recursive delete on `IOException` / `UnauthorizedAccessException` at 200/400/800 ms — a ~1.4 s total budget tuned for OneDrive / AV / Search handle release latency. Applied to all four recursive-delete sites in `UpdateInstaller` (`CleanupOldInstall`, `SwapAsync`'s `.old/` reset, `ResetStaging`, `TryDeleteDirectory`). `CleanupOldInstall` still swallows on final failure (non-fatal — next startup will retry); the other three still throw (callers depend on the install being able to fail loudly). +- Test seam: the helper accepts injectable `deleter` and `sleeper` callbacks so unit tests can simulate transient sharing-violations without a real Windows lock. + +### Why a retry rather than detecting OneDrive + +OneDrive detection is fragile (registry queries, reparse-point sniffing) and the retry strategy generalises to antivirus, Windows Search, indexers, backup agents — anything that opens a transient handle on a freshly-moved file. The cost when no contention exists is one no-op attribute walk over a tree we're about to delete: negligible. + +--- + ## [0.1.5] — 2026-05-23 ### Fixed @@ -97,6 +111,7 @@ Initial commit. Never published to nuget.org — superseded by 0.1.1 before the - Full XML documentation on the public surface, `TreatWarningsAsErrors=true`, `AnalysisLevel=latest`. - SourceLink, deterministic builds, published symbol packages. +[0.1.6]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.SelfUpdate/releases/tag/v0.1.6 [0.1.5]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.SelfUpdate/releases/tag/v0.1.5 [0.1.4]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.SelfUpdate/releases/tag/v0.1.4 [0.1.3]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.SelfUpdate/releases/tag/v0.1.3 diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj index 31d900f..66d3dce 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj +++ b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj @@ -11,7 +11,7 @@ NextIteration.SpectreConsole.SelfUpdate - 0.1.5 + 0.1.6 Stuart Meeks Self-update for Spectre.Console CLIs: pluggable update sources (GitHub Releases over HTTP, GitHub Releases via gh CLI for private repos, generic HTTPS manifest, custom), SHA-256 verification, atomic file swap, and a drop-in `update` command. true diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs index d6dc24e..e641085 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs @@ -130,15 +130,16 @@ public void CleanupOldInstall() { try { - var oldDir = Path.Combine(InstallDirectory, OldDirName); - if (Directory.Exists(oldDir)) - { - Directory.Delete(oldDir, recursive: true); - } + DeleteDirectoryRobustly(Path.Combine(InstallDirectory, OldDirName)); } catch { - // Non-fatal — will retry on next startup. + // Non-fatal — will retry on next startup. Stragglers in + // .old/ are typically the result of OneDrive / antivirus + // / Windows Search holding transient handles on files + // the swap moved in seconds earlier; the helper's retry + // budget covers the common case but a stubborn handle + // can still defeat it. } } @@ -188,11 +189,10 @@ internal static async Task SwapAsync( Func>? onConflict, CancellationToken ct) { - // Reset .old/ — it should be empty going in. - if (Directory.Exists(oldDirectory)) - { - Directory.Delete(oldDirectory, recursive: true); - } + // Reset .old/ — it should be empty going in. Stragglers from a + // previous install's CleanupOldInstall (e.g. OneDrive held a + // sync handle) get cleared here with the same retry budget. + DeleteDirectoryRobustly(oldDirectory); Directory.CreateDirectory(oldDirectory); // Phase 1: move install/ contents (except maintenance and @@ -375,7 +375,7 @@ private static void TryDeleteEntry(string path) try { if (File.Exists(path)) File.Delete(path); - else if (Directory.Exists(path)) Directory.Delete(path, recursive: true); + else if (Directory.Exists(path)) DeleteDirectoryRobustly(path); } catch { @@ -383,6 +383,74 @@ private static void TryDeleteEntry(string path) } } + /// + /// Recursive with two + /// Windows-aware adjustments: (1) clears the ReadOnly + /// attribute on every descendant file up front so the delete doesn't + /// bail with on + /// archive-extracted or VCS-checked-out content, and (2) retries on + /// / + /// at 200/400/800 ms — covers transient sharing-violations from + /// OneDrive, Windows Search, antivirus, etc. that hold short-lived + /// handles on files that were just moved into the directory. + /// + /// Directory to delete. No-op if missing. + /// Test seam — production passes to use the real . + /// Test seam — production passes to use . + internal static void DeleteDirectoryRobustly( + string path, + Action? deleter = null, + Action? sleeper = null) + { + if (!Directory.Exists(path)) return; + + deleter ??= Directory.Delete; + sleeper ??= Thread.Sleep; + + ReadOnlySpan backoffs = + [ + TimeSpan.FromMilliseconds(200), + TimeSpan.FromMilliseconds(400), + TimeSpan.FromMilliseconds(800), + ]; + + for (var attempt = 0; attempt <= backoffs.Length; attempt++) + { + ClearReadOnlyAttributes(path); + try + { + deleter(path, true); + return; + } + catch (Exception ex) when (attempt < backoffs.Length + && (ex is IOException || ex is UnauthorizedAccessException)) + { + sleeper(backoffs[attempt]); + } + } + } + + private static void ClearReadOnlyAttributes(string path) + { + try + { + foreach (var file in Directory.EnumerateFiles(path, "*", SearchOption.AllDirectories)) + { + var attrs = File.GetAttributes(file); + if ((attrs & FileAttributes.ReadOnly) != 0) + { + File.SetAttributes(file, attrs & ~FileAttributes.ReadOnly); + } + } + } + catch + { + // Best effort — if we can't clear an attribute (e.g. the file + // is locked), the subsequent Delete will surface the real + // failure and the retry loop will get another shot. + } + } + internal static bool IsMaintenanceEntry(string name) => string.Equals(name, StagingDirName, StringComparison.OrdinalIgnoreCase) || string.Equals(name, OldDirName, StringComparison.OrdinalIgnoreCase) @@ -450,7 +518,7 @@ private static void ResetStaging(string stagingDir) { try { - if (Directory.Exists(stagingDir)) Directory.Delete(stagingDir, recursive: true); + DeleteDirectoryRobustly(stagingDir); } catch (Exception ex) { @@ -464,7 +532,7 @@ private static void TryDeleteDirectory(string path) { try { - if (Directory.Exists(path)) Directory.Delete(path, recursive: true); + DeleteDirectoryRobustly(path); } catch { diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs index b8f2aca..d9819cc 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs @@ -552,6 +552,122 @@ private static RemoteRelease BuildReleaseWithSingleAssetZip( PublishedAt: DateTimeOffset.UtcNow); } + [Fact] + public void DeleteDirectoryRobustly_when_path_missing_is_noop() + { + using var work = new TempDir(); + var missing = Path.Combine(work.Path, "does-not-exist"); + + var deleterCalls = 0; + var sleeperCalls = 0; + UpdateInstaller.DeleteDirectoryRobustly( + missing, + deleter: (_, _) => deleterCalls++, + sleeper: _ => sleeperCalls++); + + Assert.Equal(0, deleterCalls); + Assert.Equal(0, sleeperCalls); + } + + [Fact] + public void DeleteDirectoryRobustly_succeeds_first_try_calls_deleter_once_no_sleeps() + { + using var work = new TempDir(); + var dir = Path.Combine(work.Path, "tree"); + Directory.CreateDirectory(dir); + + var deleterCalls = 0; + var sleeps = new List(); + UpdateInstaller.DeleteDirectoryRobustly( + dir, + deleter: (path, recursive) => + { + deleterCalls++; + Directory.Delete(path, recursive); + }, + sleeper: sleeps.Add); + + Assert.Equal(1, deleterCalls); + Assert.Empty(sleeps); + Assert.False(Directory.Exists(dir)); + } + + [Fact] + public void DeleteDirectoryRobustly_retries_with_backoff_on_transient_io_failure() + { + // Simulates OneDrive / antivirus / Windows Search holding a + // sharing-violation handle that releases within a few hundred ms. + using var work = new TempDir(); + var dir = Path.Combine(work.Path, "tree"); + Directory.CreateDirectory(dir); + + var deleterCalls = 0; + var sleeps = new List(); + UpdateInstaller.DeleteDirectoryRobustly( + dir, + deleter: (path, recursive) => + { + deleterCalls++; + if (deleterCalls < 3) + { + throw new IOException("The process cannot access the file because it is being used by another process."); + } + Directory.Delete(path, recursive); + }, + sleeper: sleeps.Add); + + Assert.Equal(3, deleterCalls); + Assert.Equal(new[] { TimeSpan.FromMilliseconds(200), TimeSpan.FromMilliseconds(400) }, sleeps); + Assert.False(Directory.Exists(dir)); + } + + [Fact] + public void DeleteDirectoryRobustly_throws_after_exhausting_retries() + { + using var work = new TempDir(); + var dir = Path.Combine(work.Path, "tree"); + Directory.CreateDirectory(dir); + + var deleterCalls = 0; + var sleeps = new List(); + var ex = Assert.Throws(() => + UpdateInstaller.DeleteDirectoryRobustly( + dir, + deleter: (_, _) => + { + deleterCalls++; + throw new IOException("stubborn handle"); + }, + sleeper: sleeps.Add)); + + Assert.Equal("stubborn handle", ex.Message); + // 1 initial attempt + 3 retries = 4 deleter calls, 3 sleeps. + Assert.Equal(4, deleterCalls); + Assert.Equal( + new[] { TimeSpan.FromMilliseconds(200), TimeSpan.FromMilliseconds(400), TimeSpan.FromMilliseconds(800) }, + sleeps); + } + + [Fact] + public void DeleteDirectoryRobustly_clears_readonly_attribute_on_descendant_files() + { + // The real .NET Directory.Delete throws UnauthorizedAccessException + // on a file marked ReadOnly. The helper has to clear that before + // delegating, otherwise extracted-archive content (which often + // arrives ReadOnly on Windows) breaks the recursive delete. + using var work = new TempDir(); + var dir = Path.Combine(work.Path, "tree"); + Directory.CreateDirectory(dir); + var readOnlyFile = Path.Combine(dir, "readonly.txt"); + File.WriteAllText(readOnlyFile, "ro"); + File.SetAttributes(readOnlyFile, File.GetAttributes(readOnlyFile) | FileAttributes.ReadOnly); + + // Use the real Directory.Delete to verify the attribute was actually cleared. + UpdateInstaller.DeleteDirectoryRobustly(dir); + + Assert.False(Directory.Exists(dir)); + } + private static byte[] CreateZipBytes((string TopFolder, (string Name, string Content)[] Files) layout) { using var ms = new MemoryStream();