diff --git a/RackPeek.Domain/Helpers/ConfigLoadException.cs b/RackPeek.Domain/Helpers/ConfigLoadException.cs new file mode 100644 index 00000000..865bb82c --- /dev/null +++ b/RackPeek.Domain/Helpers/ConfigLoadException.cs @@ -0,0 +1,18 @@ +namespace RackPeek.Domain.Helpers; + +/// +/// 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). +/// +public sealed class ConfigLoadException : Exception { + public ConfigLoadException(string message) + : base(message) { + } + + public ConfigLoadException(string message, Exception innerException) + : base(message, innerException) { + } +} diff --git a/RackPeek.Domain/Persistence/Yaml/ITextFileStore.cs b/RackPeek.Domain/Persistence/Yaml/ITextFileStore.cs index 80d1212b..02fd534a 100644 --- a/RackPeek.Domain/Persistence/Yaml/ITextFileStore.cs +++ b/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 ReadAllTextAsync(string path) => File.ReadAllTextAsync(path); - public Task WriteAllTextAsync(string path, string contents) => File.WriteAllTextAsync(path, contents); + /// + /// 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. + /// + 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; + } + } } diff --git a/RackPeek.Domain/Persistence/Yaml/YamlResourceCollection.cs b/RackPeek.Domain/Persistence/Yaml/YamlResourceCollection.cs index 8a6b62a1..747528eb 100644 --- a/RackPeek.Domain/Persistence/Yaml/YamlResourceCollection.cs +++ b/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. /// public bool Loaded { get; set; } + + /// + /// 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). + /// + 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 Exists(string name) { + ThrowIfLoadFailed(); return Task.FromResult(resourceCollection.Resources.Exists(r => r.Name.Equals(name, StringComparison.OrdinalIgnoreCase))); } public Task GetKind(string? name) { + ThrowIfLoadFailed(); return Task.FromResult(resourceCollection.Resources.FirstOrDefault(r => r.Name.Equals(name, StringComparison.OrdinalIgnoreCase))?.Kind); } public Task> 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 Exists(string name) { } public Task> GetLabelsAsync() { + ThrowIfLoadFailed(); var result = resourceCollection.Resources .SelectMany(r => r.Labels ?? Enumerable.Empty>()) .Where(kvp => !string.IsNullOrWhiteSpace(kvp.Key)) @@ -75,6 +88,7 @@ public Task> GetLabelsAsync() { } public Task> GetResourceIpsAsync() { + ThrowIfLoadFailed(); var result = new List<(Resource, string)>(); List allResources = resourceCollection.Resources; @@ -108,6 +122,7 @@ public Task> GetLabelsAsync() { } public Task> 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> GetTagsAsync() { return Task.FromResult(result); } - public Task> GetAllOfTypeAsync() => - Task.FromResult>(resourceCollection.Resources.OfType().ToList()); + public Task> GetAllOfTypeAsync() { + ThrowIfLoadFailed(); + return Task.FromResult>(resourceCollection.Resources.OfType().ToList()); + } public Task> GetDependantsAsync(string name) { + ThrowIfLoadFailed(); var result = resourceCollection.Resources .Where(r => r.RunsOn.Any(p => p.Equals(name, StringComparison.OrdinalIgnoreCase))) .ToList(); @@ -177,6 +195,7 @@ public async Task Merge(string incomingYaml, MergeMode mode) { } public Task> GetByTagAsync(string name) { + ThrowIfLoadFailed(); return Task.FromResult>( resourceCollection.Resources .Where(r => r.Tags.Contains(name)) @@ -184,27 +203,42 @@ public Task> GetByTagAsync(string name) { ); } - public IReadOnlyList HardwareResources => - resourceCollection.Resources.OfType().ToList(); + public IReadOnlyList HardwareResources { + get { + ThrowIfLoadFailed(); + return resourceCollection.Resources.OfType().ToList(); + } + } - public IReadOnlyList SystemResources => - resourceCollection.Resources.OfType().ToList(); + public IReadOnlyList SystemResources { + get { + ThrowIfLoadFailed(); + return resourceCollection.Resources.OfType().ToList(); + } + } - public IReadOnlyList ServiceResources => - resourceCollection.Resources.OfType().ToList(); + public IReadOnlyList ServiceResources { + get { + ThrowIfLoadFailed(); + return resourceCollection.Resources.OfType().ToList(); + } + } public Task GetByNameAsync(string name) { + ThrowIfLoadFailed(); return Task.FromResult(resourceCollection.Resources.FirstOrDefault(r => r.Name.Equals(name, StringComparison.OrdinalIgnoreCase))); } public Task GetByNameAsync(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; } + /// + /// 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). + /// + private void ThrowIfLoadFailed() { + if (resourceCollection.LoadFailure != null) + throw resourceCollection.LoadFailure; + } + /// /// 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> GetConnectionsAsync() { + ThrowIfLoadFailed(); IReadOnlyList result = resourceCollection.Connections .ToList() @@ -311,6 +389,7 @@ public Task> GetConnectionsAsync() { } public Task> GetConnectionsForResourceAsync(string resource) { + ThrowIfLoadFailed(); IReadOnlyList result = resourceCollection.Connections .Where(c => @@ -323,6 +402,7 @@ public Task> GetConnectionsForResourceAsync(string res } public Task GetConnectionForPortAsync(PortReference port) { + ThrowIfLoadFailed(); Connection? connection = resourceCollection.Connections .FirstOrDefault(c => diff --git a/RackPeek.Mcp/ToolErrors.cs b/RackPeek.Mcp/ToolErrors.cs index b834249c..03b08f99 100644 --- a/RackPeek.Mcp/ToolErrors.cs +++ b/RackPeek.Mcp/ToolErrors.cs @@ -21,6 +21,11 @@ public static async Task RunAsync(Func> 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); } diff --git a/RackPeek.Web.Viewer/App.razor b/RackPeek.Web.Viewer/App.razor index 2354a8a9..faa9baf3 100644 --- a/RackPeek.Web.Viewer/App.razor +++ b/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; } diff --git a/RackPeek.Web/Components/Routes.razor b/RackPeek.Web/Components/Routes.razor index 34f1b59b..40291b79 100644 --- a/RackPeek.Web/Components/Routes.razor +++ b/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; } diff --git a/Shared.Rcl/CliBootstrap.cs b/Shared.Rcl/CliBootstrap.cs index 032e0075..fa120baf 100644 --- a/Shared.Rcl/CliBootstrap.cs +++ b/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(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}"); diff --git a/Shared.Rcl/YamlFileComponent.razor b/Shared.Rcl/YamlFileComponent.razor index defc6595..26f08798 100644 --- a/Shared.Rcl/YamlFileComponent.razor +++ b/Shared.Rcl/YamlFileComponent.razor @@ -168,7 +168,18 @@ await FileStore.WriteAllTextAsync(Path, _editText); - await Resources.LoadAsync(); + try + { + await Resources.LoadAsync(); + } + catch (RackPeek.Domain.Helpers.ConfigLoadException ex) + { + // The edit was saved but the app still cannot read it. Report it here + // rather than tearing down the circuit — this editor is the repair tool. + _error = new YamlEditError(ex.Message, null, null, null); + _currentText = _editText; + return; + } _currentText = _editText; _isEditing = false; diff --git a/Shared.Rcl/wwwroot/raw_docs/install-guide.md b/Shared.Rcl/wwwroot/raw_docs/install-guide.md index dbaea7bd..1e8c7f78 100644 --- a/Shared.Rcl/wwwroot/raw_docs/install-guide.md +++ b/Shared.Rcl/wwwroot/raw_docs/install-guide.md @@ -8,6 +8,12 @@ RackPeek can run in two ways: RackPeek stores everything in a writable `config/` directory as YAML (including automatic backups). Wherever you run it, that directory must be writable. +Saves are atomic: the config is written to a temporary file, flushed to disk, then renamed +over `config.yaml`. An interrupted save therefore leaves the previous config intact rather +than a half-written one. If the config ever does become unreadable — a damaged disk, a bad +hand edit, a sync conflict — RackPeek refuses to read or write it rather than reporting an +empty inventory, and the Web UI's YAML editor (`/yaml`) still loads so you can repair it. + --- # Docker (Recommended) diff --git a/Tests.E2e/DamagedConfigTests.cs b/Tests.E2e/DamagedConfigTests.cs new file mode 100644 index 00000000..f95d98c9 --- /dev/null +++ b/Tests.E2e/DamagedConfigTests.cs @@ -0,0 +1,113 @@ +using Microsoft.Playwright; +using Tests.E2e.Infra; +using Xunit.Abstractions; + +namespace Tests.E2e; + +/// +/// Web half of https://github.com/Timmoth/RackPeek/issues/337. A config that +/// exists but cannot be read must not leave the app stuck on "Loading…" — the +/// YAML editor is how someone repairs it — and must never be reported as an +/// empty inventory. These tests stage a damaged config in the container, then +/// repair it through the UI. +/// +public class DamagedConfigTests( + PlaywrightFixture fixture, + ITestOutputHelper output) : E2ETestBase(fixture, output) { + private readonly PlaywrightFixture _fixture = fixture; + private readonly ITestOutputHelper _output = output; + + // Cut mid-token, exactly as an interrupted in-place write would leave it. + private const string _damaged = """ + version: 4 + resources: + - kind: Server + name: srv-a + - ki + """; + + private const string _healthy = """ + version: 4 + resources: + - kind: Server + name: repaired-srv + connections: [] + """; + + [Fact] + public async Task The_App_Still_Loads_And_Can_Repair_A_Damaged_Config() { + (IBrowserContext context, IPage page) = await CreatePageAsync(); + + try { + await _fixture.WriteConfigAsync(_damaged); + + // 1. The app renders rather than hanging on "Loading…". + await page.GotoAsync($"{_fixture.BaseUrl}/yaml"); + + await Assertions.Expect(page.GetByTestId("circuit-probe")) + .ToHaveAttributeAsync("data-circuit-ready", "true"); + + // 2. The editor shows the damaged file, so it can be fixed in place. + ILocator content = page.GetByTestId("yaml-file-content"); + await Assertions.Expect(content).ToBeVisibleAsync(); + await Assertions.Expect(content).ToContainTextAsync("srv-a"); + + // 3. Repair it through the editor. + await page.GetByRole(AriaRole.Button, new() { Name = "Edit" }).ClickAsync(); + + ILocator textarea = page.Locator("textarea"); + await Assertions.Expect(textarea).ToBeVisibleAsync(); + await textarea.FillAsync(_healthy); + + await page.GetByRole(AriaRole.Button, new() { Name = "Save" }).ClickAsync(); + + await Assertions.Expect(page.GetByTestId("yaml-file-error")).ToHaveCountAsync(0); + + // 4. The inventory reads correctly again. + await page.GotoAsync($"{_fixture.BaseUrl}/servers/list"); + await Assertions.Expect(page.GetByText("repaired-srv").First).ToBeVisibleAsync(); + + Assert.Contains("repaired-srv", await _fixture.ReadConfigAsync()); + } + catch (Exception) { + _output.WriteLine($"TEST FAILED — URL: {page.Url}"); + _output.WriteLine(await page.ContentAsync()); + throw; + } + finally { + // Leave the container usable for any other test in this class. + await _fixture.WriteConfigAsync(_healthy); + await context.CloseAsync(); + } + } + + [Fact] + public async Task A_Damaged_Config_Is_Never_Reported_As_An_Empty_Inventory() { + (IBrowserContext context, IPage page) = await CreatePageAsync(); + + try { + await _fixture.WriteConfigAsync(_damaged); + + await page.GotoAsync($"{_fixture.BaseUrl}/servers/list"); + + // The inventory pages read through the collection, which now refuses a + // config it could not parse. What must never happen is the page + // rendering a confident, empty list over a recoverable file. + var body = await page.InnerTextAsync("body"); + Assert.DoesNotContain("srv-a", body); + Assert.DoesNotContain("No servers", body, StringComparison.OrdinalIgnoreCase); + + // And the damaged file is still on disk, untouched by the failed read. + Assert.Equal(_damaged, (await _fixture.ReadConfigAsync()).TrimEnd('\n')); + } + catch (Exception) { + _output.WriteLine($"TEST FAILED — URL: {page.Url}"); + _output.WriteLine(await page.ContentAsync()); + throw; + } + finally { + await _fixture.WriteConfigAsync(_healthy); + await context.CloseAsync(); + } + } +} diff --git a/Tests.E2e/Infra/PlaywrightFixture.cs b/Tests.E2e/Infra/PlaywrightFixture.cs index 94d6d8e0..4c494441 100644 --- a/Tests.E2e/Infra/PlaywrightFixture.cs +++ b/Tests.E2e/Infra/PlaywrightFixture.cs @@ -1,3 +1,4 @@ +using System.Text; using DotNet.Testcontainers.Builders; using DotNet.Testcontainers.Containers; using Microsoft.Playwright; @@ -46,6 +47,22 @@ public async Task InitializeAsync() { Assertions.SetDefaultExpectTimeout(15000); } + /// + /// Replaces the container's config.yaml wholesale. Used to stage a damaged + /// config, which cannot be produced through the UI (the editor validates + /// before saving) but is exactly what an interrupted write leaves behind. + /// + public async Task WriteConfigAsync(string contents) { + await _container.CopyAsync( + Encoding.UTF8.GetBytes(contents), + "/app/config/config.yaml"); + } + + public async Task ReadConfigAsync() { + var bytes = await _container.ReadFileAsync("/app/config/config.yaml"); + return Encoding.UTF8.GetString(bytes); + } + public async Task DisposeAsync() { if (Browser != null) await Browser.DisposeAsync(); diff --git a/Tests/EndToEnd/CorruptConfigTests.cs b/Tests/EndToEnd/CorruptConfigTests.cs new file mode 100644 index 00000000..bbbf1969 --- /dev/null +++ b/Tests/EndToEnd/CorruptConfigTests.cs @@ -0,0 +1,128 @@ +using Tests.EndToEnd.Infra; +using Xunit.Abstractions; + +namespace Tests.EndToEnd; + +// Load-side half of https://github.com/Timmoth/RackPeek/issues/337. +// +// Before the fix a damaged config was accepted without complaint: an unparseable +// file loaded as an EMPTY inventory with exit 0, and the next write persisted that +// emptiness over a recoverable file. These tests pin the contract that a config +// which exists but cannot be understood fails loudly on every read, and — the part +// that actually loses data — is never overwritten. +// +// Note on what is NOT testable here: a save interrupted at a clean resource boundary +// leaves valid YAML that is indistinguishable from a smaller inventory. Nothing at +// load time can detect it, which is precisely why the writer-side fix (atomic, +// durable saves — see PhysicalTextFileStoreTests) is the primary remedy. +[Collection("Yaml CLI tests")] +public class CorruptConfigTests(TempYamlCliFixture fs, ITestOutputHelper outputHelper) + : IClassFixture { + // Exactly what the serializer writes for three bare servers. + private const string _fullConfig = """ + version: 4 + resources: + - kind: Server + name: srv-a + - kind: Server + name: srv-b + - kind: Server + name: srv-c + connections: [] + + """; + + private async Task ExecuteAsync(params string[] args) { + outputHelper.WriteLine($"rpk {string.Join(" ", args)}"); + + var output = await YamlCliTestHost.RunAsync( + args, + fs.Root, + outputHelper, + "config.yaml"); + + outputHelper.WriteLine(output); + return output; + } + + private string ConfigPath => Path.Combine(fs.Root, "config.yaml"); + + [Fact] + public async Task a_config_cut_mid_token_fails_with_a_friendly_error() { + // A save that died mid-write inside a YAML token. + var truncated = _fullConfig[.._fullConfig.IndexOf("nd: Server\n name: srv-c", StringComparison.Ordinal)]; + await File.WriteAllTextAsync(ConfigPath, truncated); + + var output = await ExecuteAsync("summary"); + + // An actionable message naming the config file — not a stack dump, and above + // all not a cheerful "Hardware (0)". + Assert.Contains("config.yaml", output); + Assert.DoesNotContain("at RackPeek.", output); + Assert.DoesNotContain("YamlDotNet.Core", output); + Assert.DoesNotContain("Hardware (0)", output); + } + + [Fact] + public async Task a_file_that_is_not_a_rackpeek_config_is_rejected() { + // Parses as YAML, carries no schema version: not our document. + await File.WriteAllTextAsync(ConfigPath, "hello: world\n"); + + var output = await ExecuteAsync("summary"); + + Assert.Contains("config.yaml", output); + Assert.DoesNotContain("Hardware (0)", output); + } + + [Fact] + public async Task a_damaged_config_is_never_overwritten_by_a_later_write() { + // The data-loss path: read the damaged file, then try to write. The write + // must refuse rather than persist the empty in-memory collection over it. + var truncated = _fullConfig[.._fullConfig.IndexOf("nd: Server\n name: srv-c", StringComparison.Ordinal)]; + await File.WriteAllTextAsync(ConfigPath, truncated); + + var output = await ExecuteAsync("servers", "add", "srv-d"); + + Assert.DoesNotContain("added", output, StringComparison.OrdinalIgnoreCase); + + var onDisk = await File.ReadAllTextAsync(ConfigPath); + Assert.Equal(truncated, onDisk); + } + + [Fact] + public async Task an_empty_config_is_still_a_valid_empty_inventory() { + // The file the CLI itself creates on first run. Must not be mistaken for damage. + await File.WriteAllTextAsync(ConfigPath, ""); + + var output = await ExecuteAsync("servers", "add", "srv-a"); + + Assert.Contains("added", output, StringComparison.OrdinalIgnoreCase); + Assert.Contains("name: srv-a", await File.ReadAllTextAsync(ConfigPath)); + } + + [Fact] + public async Task a_legacy_config_without_a_version_key_still_migrates() { + // Pre-v1 files carry no version key at all. The migration chain stamps one, + // so they must not trip the "missing schema version" guard. + await File.WriteAllTextAsync( + ConfigPath, + "resources:\n - kind: Server\n name: legacy-srv\n"); + + var output = await ExecuteAsync("summary"); + + Assert.Contains("Server: 1", output); + Assert.Contains("version: 4", await File.ReadAllTextAsync(ConfigPath)); + } + + [Fact] + public async Task a_healthy_config_still_loads_and_writes() { + await File.WriteAllTextAsync(ConfigPath, _fullConfig); + + var output = await ExecuteAsync("summary"); + Assert.Contains("Server: 3", output); + + output = await ExecuteAsync("servers", "add", "srv-d"); + Assert.Contains("added", output, StringComparison.OrdinalIgnoreCase); + Assert.Contains("name: srv-d", await File.ReadAllTextAsync(ConfigPath)); + } +} diff --git a/Tests/Yaml/PhysicalTextFileStoreTests.cs b/Tests/Yaml/PhysicalTextFileStoreTests.cs new file mode 100644 index 00000000..c2295b21 --- /dev/null +++ b/Tests/Yaml/PhysicalTextFileStoreTests.cs @@ -0,0 +1,131 @@ +using RackPeek.Domain.Persistence.Yaml; + +namespace Tests.Yaml; + +/// +/// Writer-side half of https://github.com/Timmoth/RackPeek/issues/337. +/// The store used to be a bare File.WriteAllTextAsync, which truncates the +/// destination before writing a byte and never flushes — so an interrupted save +/// could leave a truncated config, and a save that had returned successfully +/// could still be lost to power failure. It now writes to a temp file, flushes +/// to disk, and renames over the destination. +/// +public class PhysicalTextFileStoreTests : IDisposable { + private readonly string _dir = Path.Combine( + Path.GetTempPath(), + "rackpeek-store-tests", + Guid.NewGuid().ToString("N")); + + private readonly PhysicalTextFileStore _store = new(); + + public PhysicalTextFileStoreTests() => Directory.CreateDirectory(_dir); + + public void Dispose() { + if (Directory.Exists(_dir)) + Directory.Delete(_dir, true); + + GC.SuppressFinalize(this); + } + + private string Path_(string name) => Path.Combine(_dir, name); + + [Fact] + public async Task writing_a_new_file_round_trips_the_content() { + var path = Path_("config.yaml"); + + await _store.WriteAllTextAsync(path, "version: 4\n"); + + Assert.Equal("version: 4\n", await _store.ReadAllTextAsync(path)); + } + + [Fact] + public async Task overwriting_replaces_the_whole_file() { + var path = Path_("config.yaml"); + + await _store.WriteAllTextAsync(path, new string('a', 4096)); + await _store.WriteAllTextAsync(path, "short"); + + // A rename replaces the file wholesale; a partial in-place write would + // leave the tail of the longer content behind. + Assert.Equal("short", await _store.ReadAllTextAsync(path)); + } + + [Fact] + public async Task writing_leaves_no_temp_files_behind() { + var path = Path_("config.yaml"); + + for (var i = 0; i < 5; i++) + await _store.WriteAllTextAsync(path, $"version: 4 # {i}\n"); + + Assert.Equal(new[] { "config.yaml" }, + Directory.GetFiles(_dir).Select(System.IO.Path.GetFileName).OrderBy(n => n).ToArray()); + } + + [Fact] + public async Task a_reader_never_observes_a_truncated_file_during_writes() { + var path = Path_("config.yaml"); + + // Two sizes, neither a prefix of the other: any partially written state is + // detectable as "not equal to either of the two valid contents". + var big = "version: 4\n" + new string('b', 200_000); + var small = "version: 4\n" + new string('s', 50_000); + + await _store.WriteAllTextAsync(path, big); + + using var cts = new CancellationTokenSource(); + + var writer = Task.Run(async () => { + for (var i = 0; i < 40; i++) + await _store.WriteAllTextAsync(path, i % 2 == 0 ? small : big); + + await cts.CancelAsync(); + }); + + var observations = 0; + + while (!cts.IsCancellationRequested) { + string seen; + + try { + seen = await File.ReadAllTextAsync(path); + } + catch (IOException) { + // The rename can momentarily deny sharing on Windows; not a torn read. + continue; + } + + observations++; + + Assert.True(seen == big || seen == small, + $"Observed a partially written config ({seen.Length} bytes; expected {big.Length} or {small.Length})."); + } + + await writer; + + Assert.True(observations > 0, "The reader never managed to sample the file."); + } + + [Fact] + public async Task a_failed_write_leaves_the_original_intact() { + // A directory standing where the temp file wants to be makes the write fail + // after the destination would have been truncated by the old implementation. + var path = Path_("config.yaml"); + await _store.WriteAllTextAsync(path, "version: 4\nresources: []\n"); + + var readOnlyDir = Path_("locked"); + Directory.CreateDirectory(readOnlyDir); + var nested = Path.Combine(readOnlyDir, "config.yaml"); + await _store.WriteAllTextAsync(nested, "version: 4\n"); + + // Writing to a path that is itself a directory always fails. + var directoryPath = Path_("a-directory"); + Directory.CreateDirectory(directoryPath); + + await Assert.ThrowsAnyAsync( + () => _store.WriteAllTextAsync(directoryPath, "anything")); + + // The unrelated config is untouched, and no temp debris was left anywhere. + Assert.Equal("version: 4\nresources: []\n", await _store.ReadAllTextAsync(path)); + Assert.DoesNotContain(Directory.GetFiles(_dir), f => f.Contains(".tmp-", StringComparison.Ordinal)); + } +}