From dd3cd6b66c15a75b4633b51eab6a7af2ca0d1346 Mon Sep 17 00:00:00 2001 From: Aiden <68633820+awils27@users.noreply.github.com> Date: Mon, 11 May 2026 20:11:20 +1000 Subject: [PATCH] Clean up --- .../runtime/persistence/PersistenceWriter.cpp | 9 ++------- .../runtime/persistence/PersistenceWriter.h | 1 - .../runtime/telemetry/HealthTelemetry.h | 5 ++--- docs/ARCHITECTURE_RESILIENCE_REVIEW.md | 17 +++++++++++------ docs/PHASE_6_BACKGROUND_PERSISTENCE_DESIGN.md | 8 ++++---- docs/subsystems/RuntimeStore.md | 11 ++++++----- tests/PersistenceWriterTests.cpp | 6 +++--- 7 files changed, 28 insertions(+), 29 deletions(-) diff --git a/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.cpp b/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.cpp index 67cfd06..5b1887b 100644 --- a/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.cpp +++ b/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.cpp @@ -15,7 +15,8 @@ PersistenceWriter::PersistenceWriter(std::chrono::milliseconds debounceDelay, Sn PersistenceWriter::~PersistenceWriter() { - StopAndFlush(); + std::string error; + StopAndFlush((std::chrono::milliseconds::max)(), error); } void PersistenceWriter::SetResultCallback(ResultCallback callback) @@ -71,12 +72,6 @@ bool PersistenceWriter::EnqueueSnapshot(const PersistenceSnapshot& snapshot, std return true; } -void PersistenceWriter::StopAndFlush() -{ - std::string error; - StopAndFlush((std::chrono::milliseconds::max)(), error); -} - bool PersistenceWriter::StopAndFlush(std::chrono::milliseconds timeout, std::string& error) { { diff --git a/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.h b/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.h index 8d65203..232ac7e 100644 --- a/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.h +++ b/apps/LoopThroughWithOpenGLCompositing/runtime/persistence/PersistenceWriter.h @@ -46,7 +46,6 @@ public: bool WriteSnapshot(const PersistenceSnapshot& snapshot, std::string& error); bool EnqueueSnapshot(const PersistenceSnapshot& snapshot, std::string& error); bool StopAndFlush(std::chrono::milliseconds timeout, std::string& error); - void StopAndFlush(); PersistenceWriterMetrics GetMetrics() const; private: diff --git a/apps/LoopThroughWithOpenGLCompositing/runtime/telemetry/HealthTelemetry.h b/apps/LoopThroughWithOpenGLCompositing/runtime/telemetry/HealthTelemetry.h index 7b4734f..3d609b9 100644 --- a/apps/LoopThroughWithOpenGLCompositing/runtime/telemetry/HealthTelemetry.h +++ b/apps/LoopThroughWithOpenGLCompositing/runtime/telemetry/HealthTelemetry.h @@ -5,9 +5,8 @@ #include #include -// Phase 1 compatibility seam for status and timing reporting. HealthTelemetry -// owns the current operational status snapshot directly, so callers can report -// health without sharing runtime-store state. +// HealthTelemetry owns the current operational status snapshot directly, so +// callers can report health without sharing runtime-store state. class HealthTelemetry { public: diff --git a/docs/ARCHITECTURE_RESILIENCE_REVIEW.md b/docs/ARCHITECTURE_RESILIENCE_REVIEW.md index c8ed28d..e482ae2 100644 --- a/docs/ARCHITECTURE_RESILIENCE_REVIEW.md +++ b/docs/ARCHITECTURE_RESILIENCE_REVIEW.md @@ -260,17 +260,22 @@ Recommended direction: ### 7. Persistence should be more asynchronous and debounced -`SavePersistentState()` is still called directly from many update paths. +Status: addressed by Phase 6. Relevant code: -- `RuntimeHost.cpp` +- `RuntimeCoordinator.cpp` +- `RuntimeUpdateController.cpp` +- `RuntimeStore.cpp` +- `PersistenceWriter.cpp` -Recent OSC work already reduced this problem for live automation, but the broader architecture would still benefit from: +Runtime-state persistence now flows from accepted coordinator mutations to typed persistence events, then into a debounced background writer. The store still owns serialization and preset IO, while the writer owns temp-file replacement, coalescing, result reporting, and shutdown flushing. -- a debounced persistence queue -- atomic write-behind snapshots -- clear separation between state mutation and disk flush +The remaining architecture concern is broader persistence policy, not direct mutation-path disk writes: + +- whether preset saves should stay synchronous +- whether runtime config writes should share the persistence writer +- whether failed writes should retry automatically or wait for the next request This improves both resilience and timing safety. diff --git a/docs/PHASE_6_BACKGROUND_PERSISTENCE_DESIGN.md b/docs/PHASE_6_BACKGROUND_PERSISTENCE_DESIGN.md index 51c8def..98f0b63 100644 --- a/docs/PHASE_6_BACKGROUND_PERSISTENCE_DESIGN.md +++ b/docs/PHASE_6_BACKGROUND_PERSISTENCE_DESIGN.md @@ -23,7 +23,7 @@ Current persistence footholds: Synchronous persistence is a poor fit for live software. A mutation that changes state should not also have to block on filesystem timing, antivirus scans, slow disks, or transient IO failures. The app needs persistence to be reliable and observable, but not timing-sensitive. -The resilience review calls this out because `SavePersistentState()` style behavior can create unnecessary stalls and makes recovery harder to reason about. +The resilience review calls this out because synchronous save-after-mutate behavior can create unnecessary stalls and makes recovery harder to reason about. Phase 6 should turn persistence into: @@ -209,8 +209,8 @@ Current implementation: - `RuntimeStore::BuildRuntimeStatePersistenceSnapshot(...)` captures serialized runtime-state content and target path. - `PersistenceWriter::WriteSnapshot(...)` owns the temp-file and replace write mechanics. -- `RuntimeStore::SavePersistentState(...)` still behaves synchronously, but now writes through `PersistenceWriter`. -- Stack preset saves also use `PersistenceWriter` synchronously; preset async policy remains a later decision. +- Runtime-state persistence now flows through `RuntimeStore::RequestPersistence(...)` and the background writer. +- Stack preset saves still use `PersistenceWriter` synchronously; preset async policy remains a later decision. ### Step 3. Add Debounced Background Worker @@ -226,7 +226,7 @@ Current implementation: - `PersistenceWriter::EnqueueSnapshot(...)` starts a worker lazily and debounces snapshots by `debounceKey`. - Runtime-state saves enqueue debounced snapshots, so routine mutation paths no longer write the runtime-state file directly. -- Synchronous `PersistenceWriter::WriteSnapshot(...)` remains for stack preset saves and transitional direct writes. +- Synchronous `PersistenceWriter::WriteSnapshot(...)` remains for stack preset saves. - `PersistenceWriterTests` use an injected in-memory sink to verify coalescing and non-coalesced immediate requests without touching the filesystem. ### Step 4. Add Atomic Write And Failure Reporting diff --git a/docs/subsystems/RuntimeStore.md b/docs/subsystems/RuntimeStore.md index f4f4435..88ebba3 100644 --- a/docs/subsystems/RuntimeStore.md +++ b/docs/subsystems/RuntimeStore.md @@ -187,7 +187,8 @@ Expected responsibilities: - `LoadConfig()` - `LoadPersistentState()` -- `SavePersistentStateSnapshot(...)` +- `BuildPersistentStateSnapshot(...)` +- `RequestPersistence(...)` - `LoadStackPreset(...)` - `SaveStackPreset(...)` - `GetStackPresetNames()` @@ -196,7 +197,7 @@ Design notes: - `Load*` operations should parse and normalize external file content into durable in-memory models. - `Save*` operations should serialize durable models without needing render or control subsystem context. -- later debounce/background writing should wrap these operations, not redefine their ownership +- debounce/background writing wraps these operations rather than redefining store ownership ### Read Interface @@ -406,14 +407,14 @@ Target behavior: - serialization snapshots are built from those models - save requests persist a coherent snapshot -This matters because the current code still calls `SavePersistentState()` directly from many mutation paths. That is one of the architectural pressure points already called out in [ARCHITECTURE_RESILIENCE_REVIEW.md](/c:/Users/Aiden/Documents/GitHub/video-shader-toys/docs/ARCHITECTURE_RESILIENCE_REVIEW.md). +This matters because earlier code called persistent-state saves directly from mutation paths. Phase 6 removed that pressure point: accepted durable mutations now publish persistence requests, and `RuntimeStore::RequestPersistence(...)` builds a coherent snapshot for the background writer. The Phase 1 design for `RuntimeStore` should therefore assume: - store ownership of serialization remains -- immediate save-after-mutate is a migration detail, not the final behavioral contract +- persistence requests, not mutation methods, are the durable write boundary -By Phase 6, a background snapshot writer may sit underneath or beside this subsystem, but the durable model still belongs here. +Phase 6 added that background snapshot writer underneath this subsystem, while keeping the durable model here. ## Migration Plan From Current Code diff --git a/tests/PersistenceWriterTests.cpp b/tests/PersistenceWriterTests.cpp index 0148c00..0734171 100644 --- a/tests/PersistenceWriterTests.cpp +++ b/tests/PersistenceWriterTests.cpp @@ -54,7 +54,7 @@ void TestDebouncedRequestsCoalesceToNewestSnapshot() Expect(metrics.enqueuedCount == 1, "first debounced snapshot counts as enqueue"); Expect(metrics.coalescedCount == 1, "second debounced snapshot counts as coalesced"); - writer.StopAndFlush(); + Expect(writer.StopAndFlush(std::chrono::seconds(1), error), "flush drains debounced snapshots"); { std::lock_guard lock(mutex); @@ -87,7 +87,7 @@ void TestImmediateRequestsAreNotCoalesced() std::string error; Expect(writer.EnqueueSnapshot(first, error), "first immediate snapshot enqueues"); Expect(writer.EnqueueSnapshot(second, error), "second immediate snapshot enqueues"); - writer.StopAndFlush(); + Expect(writer.StopAndFlush(std::chrono::seconds(1), error), "flush drains immediate snapshots"); { std::lock_guard lock(mutex); @@ -116,7 +116,7 @@ void TestWriteFailureReportsStructuredResult() std::string error; Expect(writer.EnqueueSnapshot(snapshot, error), "failing snapshot still enqueues"); - writer.StopAndFlush(); + Expect(writer.StopAndFlush(std::chrono::seconds(1), error), "flush reports failing snapshot result"); Expect(results.size() == 1, "writer reports one failure result"); Expect(!results.empty() && !results[0].succeeded, "writer result records failure");