Make config saves atomic and refuse to read a damaged config (#337)

RackPeek.Domain/Helpers/ConfigLoadException.cs

@@ -0,0 +1,18 @@
+namespace RackPeek.Domain.Helpers;
+
+/// <summary>
+///     The config file exists but cannot be read as a RackPeek document — damaged,
+///     truncated, or not YAML. Distinct from an unreadable store (IO errors), which is
+///     tolerated at boot: a damaged file must fail loudly on every read and write so a
+///     partial or empty inventory is never served, and never persisted over the
+///     user's file (#337).
+/// </summary>
+public sealed class ConfigLoadException : Exception {
+    public ConfigLoadException(string message)
+        : base(message) {
+    }
+
+    public ConfigLoadException(string message, Exception innerException)
+        : base(message, innerException) {
+    }
+}

RackPeek.Domain/Persistence/Yaml/ITextFileStore.cs

@@ -1,3 +1,5 @@
+using System.Text;
+
 namespace RackPeek.Domain.Persistence.Yaml;
 
 public interface ITextFileStore {
@@ -11,5 +13,51 @@ public sealed class PhysicalTextFileStore : ITextFileStore {
 
     public Task<string> ReadAllTextAsync(string path) => File.ReadAllTextAsync(path);
 
-    public Task WriteAllTextAsync(string path, string contents) => File.WriteAllTextAsync(path, contents);
+    /// <summary>
+    ///     Atomic and durable replacement for File.WriteAllTextAsync, which truncates
+    ///     the destination before writing and never flushes to disk — so a crash or
+    ///     power loss mid-save could leave a truncated config, and a save that had
+    ///     "succeeded" could still be lost (#337). The content is written to a
+    ///     temporary file in the same directory, flushed to disk, then moved over the
+    ///     destination — a rename, so readers only ever see the old or the new file,
+    ///     never a partial one.
+    /// </summary>
+    public async Task WriteAllTextAsync(string path, string contents) {
+        var fullPath = Path.GetFullPath(path);
+        var directory = Path.GetDirectoryName(fullPath)
+                        ?? throw new IOException($"'{path}' has no parent directory.");
+
+        var tempPath = Path.Combine(
+            directory,
+            $"{Path.GetFileName(fullPath)}.tmp-{Guid.NewGuid():N}");
+
+        try {
+            await using (var stream = new FileStream(
+                             tempPath,
+                             FileMode.CreateNew,
+                             FileAccess.Write,
+                             FileShare.None)) {
+                var bytes = Encoding.UTF8.GetBytes(contents);
+                await stream.WriteAsync(bytes);
+
+                // Flush through the OS cache to the disk itself, so the rename
+                // below never publishes a file whose bytes could still vanish.
+                stream.Flush(true);
+            }
+
+            File.Move(tempPath, fullPath, true);
+        }
+        catch {
+            // Never leave temp files behind on a failed write; the destination
+            // is untouched by construction.
+            try {
+                File.Delete(tempPath);
+            }
+            catch (IOException) {
+                // Best effort — the stray temp file is harmless.
+            }
+
+            throw;
+        }
+    }
 }

RackPeek.Domain/Persistence/Yaml/YamlResourceCollection.cs

@@ -2,6 +2,7 @@
 using System.Collections.Specialized;
 using System.Diagnostics;
 using RackPeek.Domain.Discovery;
+using RackPeek.Domain.Helpers;
 using RackPeek.Domain.Resources;
 using RackPeek.Domain.Resources.AccessPoints;
 using RackPeek.Domain.Resources.Connections;
@@ -33,6 +34,14 @@ public class ResourceCollection {
     ///     the user's file.
     /// </summary>
     public bool Loaded { get; set; }
+
+    /// <summary>
+    ///     Set when the config exists but could not be understood — damaged, truncated,
+    ///     or not YAML. The process is still allowed to boot (the web UI is how someone
+    ///     fixes the file), but every read and write must fail loudly rather than serve
+    ///     or persist an empty inventory (#337).
+    /// </summary>
+    public ConfigLoadException? LoadFailure { get; set; }
 }
 
 public sealed class YamlResourceCollection(
@@ -45,16 +54,19 @@ public sealed class YamlResourceCollection(
     private static readonly int _currentSchemaVersion = RackPeekConfigMigrationDeserializer.ListOfMigrations.Count;
 
     public Task<bool> Exists(string name) {
+        ThrowIfLoadFailed();
         return Task.FromResult(resourceCollection.Resources.Exists(r =>
             r.Name.Equals(name, StringComparison.OrdinalIgnoreCase)));
     }
 
     public Task<string?> GetKind(string? name) {
+        ThrowIfLoadFailed();
         return Task.FromResult(resourceCollection.Resources.FirstOrDefault(r =>
             r.Name.Equals(name, StringComparison.OrdinalIgnoreCase))?.Kind);
     }
 
     public Task<IReadOnlyList<(Resource, string)>> GetByLabelAsync(string name) {
+        ThrowIfLoadFailed();
         ReadOnlyCollection<(Resource r, string)> result = resourceCollection.Resources
             .Where(r => r.Labels != null && r.Labels.TryGetValue(name, out _))
             .Select(r => (r, r.Labels![name]))
@@ -65,6 +77,7 @@ public Task<bool> Exists(string name) {
     }
 
     public Task<Dictionary<string, int>> GetLabelsAsync() {
+        ThrowIfLoadFailed();
         var result = resourceCollection.Resources
             .SelectMany(r => r.Labels ?? Enumerable.Empty<KeyValuePair<string, string>>())
             .Where(kvp => !string.IsNullOrWhiteSpace(kvp.Key))
@@ -75,6 +88,7 @@ public Task<Dictionary<string, int>> GetLabelsAsync() {
     }
 
     public Task<IReadOnlyList<(Resource, string)>> GetResourceIpsAsync() {
+        ThrowIfLoadFailed();
         var result = new List<(Resource, string)>();
 
         List<Resource> allResources = resourceCollection.Resources;
@@ -108,6 +122,7 @@ public Task<Dictionary<string, int>> GetLabelsAsync() {
     }
 
     public Task<Dictionary<string, int>> GetTagsAsync() {
+        ThrowIfLoadFailed();
         var result = resourceCollection.Resources
             .SelectMany(r => r.Tags) // flatten all tag arrays
             .Where(t => !string.IsNullOrWhiteSpace(t))
@@ -117,10 +132,13 @@ public Task<Dictionary<string, int>> GetTagsAsync() {
         return Task.FromResult(result);
     }
 
-    public Task<IReadOnlyList<T>> GetAllOfTypeAsync<T>() =>
-        Task.FromResult<IReadOnlyList<T>>(resourceCollection.Resources.OfType<T>().ToList());
+    public Task<IReadOnlyList<T>> GetAllOfTypeAsync<T>() {
+        ThrowIfLoadFailed();
+        return Task.FromResult<IReadOnlyList<T>>(resourceCollection.Resources.OfType<T>().ToList());
+    }
 
     public Task<IReadOnlyList<Resource>> GetDependantsAsync(string name) {
+        ThrowIfLoadFailed();
         var result = resourceCollection.Resources
             .Where(r => r.RunsOn.Any(p => p.Equals(name, StringComparison.OrdinalIgnoreCase)))
             .ToList();
@@ -177,34 +195,50 @@ public async Task Merge(string incomingYaml, MergeMode mode) {
     }
 
     public Task<IReadOnlyList<Resource>> GetByTagAsync(string name) {
+        ThrowIfLoadFailed();
         return Task.FromResult<IReadOnlyList<Resource>>(
             resourceCollection.Resources
                 .Where(r => r.Tags.Contains(name))
                 .ToList()
         );
     }
 
-    public IReadOnlyList<Hardware> HardwareResources =>
-        resourceCollection.Resources.OfType<Hardware>().ToList();
+    public IReadOnlyList<Hardware> HardwareResources {
+        get {
+            ThrowIfLoadFailed();
+            return resourceCollection.Resources.OfType<Hardware>().ToList();
+        }
+    }
 
-    public IReadOnlyList<SystemResource> SystemResources =>
-        resourceCollection.Resources.OfType<SystemResource>().ToList();
+    public IReadOnlyList<SystemResource> SystemResources {
+        get {
+            ThrowIfLoadFailed();
+            return resourceCollection.Resources.OfType<SystemResource>().ToList();
+        }
+    }
 
-    public IReadOnlyList<Service> ServiceResources =>
-        resourceCollection.Resources.OfType<Service>().ToList();
+    public IReadOnlyList<Service> ServiceResources {
+        get {
+            ThrowIfLoadFailed();
+            return resourceCollection.Resources.OfType<Service>().ToList();
+        }
+    }
 
     public Task<Resource?> GetByNameAsync(string name) {
+        ThrowIfLoadFailed();
         return Task.FromResult(resourceCollection.Resources.FirstOrDefault(r =>
             r.Name.Equals(name, StringComparison.OrdinalIgnoreCase)));
     }
 
     public Task<T?> GetByNameAsync<T>(string name) where T : Resource {
+        ThrowIfLoadFailed();
         Resource? resource =
             resourceCollection.Resources.FirstOrDefault(r => r.Name.Equals(name, StringComparison.OrdinalIgnoreCase));
         return Task.FromResult(resource as T);
     }
 
     public Resource? GetByName(string name) {
+        ThrowIfLoadFailed();
         return resourceCollection.Resources.FirstOrDefault(r =>
             r.Name.Equals(name, StringComparison.OrdinalIgnoreCase));
     }
@@ -227,11 +261,42 @@ public async Task LoadAsync() {
     private async Task LoadUnderLockAsync() {
         var yaml = await fileStore.ReadAllTextAsync(filePath);
 
-        YamlRoot root = await migrationService.DeserializeAsync(
-            yaml,
-            async originalYaml => await BackupOriginalAsync(originalYaml),
-            async migratedRoot => await SaveRootAsync(migratedRoot)
-        );
+        YamlRoot root;
+
+        try {
+            root = await migrationService.DeserializeAsync(
+                yaml,
+                async originalYaml => await BackupOriginalAsync(originalYaml),
+                async migratedRoot => await SaveRootAsync(migratedRoot)
+            );
+        }
+        catch (Exception ex) when (ex is not ConfigLoadException
+                                       and not IOException
+                                       and not UnauthorizedAccessException) {
+            // A file that exists but cannot be understood. Record it so that every
+            // later read and write refuses, rather than quietly serving — and then
+            // persisting — an empty inventory over a recoverable file (#337).
+            resourceCollection.LoadFailure = new ConfigLoadException(
+                $"The config at {filePath} could not be read: {ex.Message} " +
+                "Fix or restore the file (recent schema migrations leave .bak copies " +
+                "beside it); nothing has been changed.",
+                ex);
+
+            throw resourceCollection.LoadFailure;
+        }
+
+        // A RackPeek document always carries a schema version. Its absence means the
+        // file was cut before the version line was written, or is not a RackPeek
+        // config at all — either way the parse "succeeding" with an empty document is
+        // not evidence of an empty inventory.
+        if (!string.IsNullOrWhiteSpace(yaml) && root.Version <= 0) {
+            resourceCollection.LoadFailure = new ConfigLoadException(
+                $"The config at {filePath} is missing its schema version, so it is " +
+                "incomplete or not a RackPeek config. Fix or restore the file; " +
+                "nothing has been changed.");
+
+            throw resourceCollection.LoadFailure;
+        }
 
         resourceCollection.Resources.Clear();
 
@@ -243,9 +308,21 @@ private async Task LoadUnderLockAsync() {
         if (root.Connections != null)
             resourceCollection.Connections.AddRange(root.Connections);
 
+        resourceCollection.LoadFailure = null;
         resourceCollection.Loaded = true;
     }
 
+    /// <summary>
+    ///     Called at the top of every read path. When the config exists but could not be
+    ///     understood, the in-memory collection is empty for a reason that has nothing to
+    ///     do with the user's inventory — serving it would report "0 resources" for a
+    ///     recoverable file, and scripted consumers would treat that as the truth (#337).
+    /// </summary>
+    private void ThrowIfLoadFailed() {
+        if (resourceCollection.LoadFailure != null)
+            throw resourceCollection.LoadFailure;
+    }
+
     /// <summary>
     ///     Called at the top of every write path, under the lock. Normally a no-op:
     ///     both the CLI and the web host load at startup. When that startup load failed
@@ -302,6 +379,7 @@ public Task RemoveConnectionsForPortAsync(PortReference port) {
     }
 
     public Task<IReadOnlyList<Connection>> GetConnectionsAsync() {
+        ThrowIfLoadFailed();
         IReadOnlyList<Connection> result =
             resourceCollection.Connections
                 .ToList()
@@ -311,6 +389,7 @@ public Task<IReadOnlyList<Connection>> GetConnectionsAsync() {
     }
 
     public Task<IReadOnlyList<Connection>> GetConnectionsForResourceAsync(string resource) {
+        ThrowIfLoadFailed();
         IReadOnlyList<Connection> result =
             resourceCollection.Connections
                 .Where(c =>
@@ -323,6 +402,7 @@ public Task<IReadOnlyList<Connection>> GetConnectionsForResourceAsync(string res
     }
 
     public Task<Connection?> GetConnectionForPortAsync(PortReference port) {
+        ThrowIfLoadFailed();
         Connection? connection =
             resourceCollection.Connections
                 .FirstOrDefault(c =>

RackPeek.Mcp/ToolErrors.cs

@@ -21,6 +21,11 @@ public static async Task<T> RunAsync<T>(Func<Task<T>> action) {
         catch (NotFoundException ex) {
             throw new McpException(ex.Message);
         }
+        catch (ConfigLoadException ex) {
+            // The config exists but cannot be read. Say so plainly rather than letting
+            // the agent see a generic failure and conclude the inventory is empty.
+            throw new McpException(ex.Message);
+        }
         catch (ConflictException ex) {
             throw new McpException(ex.Message);
         }

RackPeek.Web.Viewer/App.razor

@@ -1,4 +1,5 @@
-@using RackPeek.Domain.Persistence
+@using RackPeek.Domain.Helpers
+@using RackPeek.Domain.Persistence
 @using RackPeek.Web.Viewer.Pages
 @using Shared.Rcl.Servers
 @inject IResourceCollection Resources
@@ -23,7 +24,17 @@ else
 
     protected override async Task OnInitializedAsync()
     {
-        await Resources.LoadAsync();
+        try
+        {
+            await Resources.LoadAsync();
+        }
+        catch (ConfigLoadException)
+        {
+            // Same contract as the server host: a config that cannot be read must not
+            // leave the app stuck on "Loading…" — the YAML editor is how it is fixed.
+            // Here the store is browser storage, so this is a bad import rather than
+            // an interrupted write (#337).
+        }
 
         _ready = true;
     }

RackPeek.Web/Components/Routes.razor

@@ -1,4 +1,5 @@
-@using RackPeek.Domain.Persistence
+@using RackPeek.Domain.Helpers
+@using RackPeek.Domain.Persistence
 @using RackPeek.Web.Components.Pages
 @using Shared.Rcl.Servers
 @inject IResourceCollection Resources
@@ -23,7 +24,16 @@ else
 
     protected override async Task OnInitializedAsync()
     {
-        await Resources.LoadAsync();
+        try
+        {
+            await Resources.LoadAsync();
+        }
+        catch (ConfigLoadException)
+        {
+            // A damaged config must not leave the app stuck on "Loading…" — the YAML
+            // editor is how someone repairs it. Pages that read the inventory surface
+            // the failure themselves; they can no longer report it as empty (#337).
+        }
 
         _ready = true;
     }

Shared.Rcl/CliBootstrap.cs

@@ -138,6 +138,13 @@ await System.Console.Error.WriteLineAsync(
             // matter and is still allowed to fail loudly — the user has one to fix.
             await System.Console.Error.WriteLineAsync($"Warning: could not read {fullYamlPath} ({ex.Message}).");
         }
+        catch (ConfigLoadException) {
+            // A damaged config must not stop the process starting — `rpk discover` and
+            // `--help` do not need the inventory, and the web UI is how someone fixes
+            // the file. The failure is recorded on the collection, so every command
+            // that does touch the inventory fails with it instead of reporting an
+            // empty one (#337).
+        }
         services.AddSingleton<IResourceCollection>(collection);
 
         // Infrastructure
@@ -879,6 +886,10 @@ private static int HandleException(Exception ex, ITypeResolver? arg2) {
                 AnsiConsole.MarkupLine($"[red]Not found:[/] {ne.Message}");
                 return 4;
 
+            case ConfigLoadException cle:
+                AnsiConsole.MarkupLine($"[red]Config error:[/] {Markup.Escape(cle.Message)}");
+                return 5;
+
             case CommandParseException pe:
                 if (_showingHelp) return 1; // suppress errors during help lookup
                 AnsiConsole.MarkupLine($"[red]Invalid command:[/] {pe.Message}");