[ADR-181] Real CloudAssetOrchestrator with cache-first load, background refresh, and file-watch loop#159
Conversation
There was a problem hiding this comment.
Pull request overview
Implements a real cloud asset orchestration pipeline for layout (cache-first startup, background refresh, and file-change driven reload), and extends unit/E2E coverage to validate startup and update behavior across cache/server/stub-file scenarios.
Changes:
- Replaces the stub
CloudAssetOrchestratorwith a multi-phase implementation (cache load → background refresh → file-change loop) plus cache + notifier abstractions. - Adds file-backed
CloudAssetCacheand a debouncedFileSystemWatcher-basedIAssetChangeNotifier. - Expands unit tests and headless E2E scenarios/fixtures to cover cache hit/miss, background refresh, file change updates, and fatal-error startup paths.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/AdaptiveRemote.EndtoEndTests.TestServices/Layout/updated-layout.json | Adds updated layout fixture (includes Guide button) for cloud-layout E2E scenarios. |
| test/AdaptiveRemote.EndtoEndTests.TestServices/Layout/primary-layout.json | Adds primary layout fixture (no Guide button) for cache/stub baseline. |
| test/AdaptiveRemote.EndtoEndTests.TestServices/Host/SimulatedEnvironment.cs | Adds cloud cache/stub path plumbing and passes cloud settings via command-line args to the host. |
| test/AdaptiveRemote.EndtoEndTests.TestServices/Host/ISimulatedEnvironment.cs | Exposes configured cloud cache/stub paths to step definitions. |
| test/AdaptiveRemote.EndtoEndTests.TestServices/ApplicationTestService.cs | Lazily resolves IRemoteDefinitionService so phase querying works in FatalError scenarios. |
| test/AdaptiveRemote.EndtoEndTests.TestServices/AdaptiveRemote.EndtoEndTests.TestServices.csproj | Copies layout JSON fixtures to output for runtime access by steps/hooks. |
| test/AdaptiveRemote.EndToEndTests.Steps/Hooks/EnvironmentSetupHooks.cs | Initializes a per-test-run stub file + cache directory and configures the simulated host accordingly. |
| test/AdaptiveRemote.EndToEndTests.Steps/CloudLayoutSteps.cs | Adds step definitions to manipulate cache/stub file and assert UI changes. |
| test/AdaptiveRemote.EndToEndTests.Host.Headless/Features/Layout/CloudLayoutUpdate.feature | Adds @cloud-layout scenarios for cache-first startup, background refresh, file updates, and fatal error behavior. |
| test/AdaptiveRemote.App.Tests/Services/CloudAssets/FileSystemCloudAssetWatchServiceTests.cs | Adds unit tests validating file change notification and debouncing behavior. |
| test/AdaptiveRemote.App.Tests/Services/CloudAssets/CloudAssetOrchestratorTests.cs | Adds extensive unit tests for cache hit/miss, background refresh, file-change loop, and idle-deferred recycle logic. |
| test/AdaptiveRemote.App.Tests/Services/CloudAssets/CloudAssetCacheTests.cs | Adds unit tests for file-backed cache save/load behavior via IFileSystem. |
| src/AdaptiveRemote.App/Services/CloudAssets/_doc_CloudAssets.md | Documents the cloud assets model, orchestrator phases, cache format, and watch service behavior. |
| src/AdaptiveRemote.App/Services/CloudAssets/IAssetChangeNotifier.cs | Introduces abstraction for change notification to decouple watcher implementation from orchestrator. |
| src/AdaptiveRemote.App/Services/CloudAssets/FileSystemCloudAssetWatchService.cs | Implements debounced FileSystemWatcher notifier as a hosted service. |
| src/AdaptiveRemote.App/Services/CloudAssets/CloudAssetOrchestrator.cs | Implements cache-first load + background refresh + file-change loop, with hashing and idle-deferred recycle. |
| src/AdaptiveRemote.App/Services/CloudAssets/CloudAssetCache.cs | Implements ICloudAssetCache storing {name}.cache under configured cache path using IFileSystem. |
| src/AdaptiveRemote.App/Logging/MessageLogger.cs | Adds structured log events for cache load, up-to-date, update/recycle, background fetch failure, and file-change detection. |
| src/AdaptiveRemote.App/Configuration/CloudAssetServiceExtensions.cs | Wires cache, downloader, watch service, and orchestrator into DI + hosted services. |
| private void IdleDeferRecycle() | ||
| { | ||
| if (_idleDetector.IsIdle) | ||
| { | ||
| _signal.RequestRecycle(); | ||
| return; | ||
| } | ||
|
|
||
| void OnBecameIdle(object? sender, EventArgs args) | ||
| { | ||
| _idleDetector.BecameIdle -= OnBecameIdle; | ||
| _signal.RequestRecycle(); | ||
| } | ||
| _idleDetector.BecameIdle += OnBecameIdle; |
There was a problem hiding this comment.
IdleDeferRecycle() subscribes a new BecameIdle handler every time it’s called while not idle. If multiple asset updates occur while the user is active, this can accumulate handlers and trigger multiple RequestRecycle() calls when BecameIdle fires. Consider tracking whether a recycle is already pending (or storing/unsubscribing a single handler) so only one subscription is active at a time.
| private void IdleDeferRecycle() | |
| { | |
| if (_idleDetector.IsIdle) | |
| { | |
| _signal.RequestRecycle(); | |
| return; | |
| } | |
| void OnBecameIdle(object? sender, EventArgs args) | |
| { | |
| _idleDetector.BecameIdle -= OnBecameIdle; | |
| _signal.RequestRecycle(); | |
| } | |
| _idleDetector.BecameIdle += OnBecameIdle; | |
| private EventHandler? _pendingRecycleHandler; | |
| private void IdleDeferRecycle() | |
| { | |
| if (_idleDetector.IsIdle) | |
| { | |
| if (_pendingRecycleHandler is not null) | |
| { | |
| _idleDetector.BecameIdle -= _pendingRecycleHandler; | |
| _pendingRecycleHandler = null; | |
| } | |
| _signal.RequestRecycle(); | |
| return; | |
| } | |
| if (_pendingRecycleHandler is not null) | |
| { | |
| return; | |
| } | |
| EventHandler handler = null!; | |
| handler = (sender, args) => | |
| { | |
| _idleDetector.BecameIdle -= handler; | |
| _pendingRecycleHandler = null; | |
| _signal.RequestRecycle(); | |
| }; | |
| _pendingRecycleHandler = handler; | |
| _idleDetector.BecameIdle += handler; |
There was a problem hiding this comment.
Fixed in 06a8715. Added a _pendingRecycleHandler field that tracks the active BecameIdle subscription. IdleDeferRecycle returns early if _pendingRecycleHandler != null, and the handler clears the field before calling RequestRecycle, so only one subscription is ever active at a time.
Generated by Claude Code
| }; | ||
|
|
||
| watcher.Changed += OnChanged; | ||
| watcher.Created += OnChanged; |
There was a problem hiding this comment.
The watcher only listens to Changed/Created events. Many editors update files via a temp-write + rename, which can raise Renamed (and sometimes Deleted) instead; those changes won’t trigger a refresh. Consider handling Renamed/Deleted as well (or using the watcher’s Error event to recreate) so stub updates are reliably detected.
| watcher.Created += OnChanged; | |
| watcher.Created += OnChanged; | |
| watcher.Renamed += OnChanged; | |
| watcher.Deleted += OnChanged; |
There was a problem hiding this comment.
Fixed in 06a8715 — added watcher.Renamed += OnChanged. Skipped Deleted: a deleted stub means there's nothing new to fetch, so the next Phase 3 poll will log a warning and keep the existing store value, which is the correct behaviour.
Generated by Claude Code
| // Wait for Phase 2 to run (store.Set called twice — once in Phase 1, once in Phase 2) | ||
| Task.Delay(TimeSpan.FromMilliseconds(200)).Wait(); | ||
|
|
There was a problem hiding this comment.
These tests rely on fixed Thread-blocking delays (e.g., Task.Delay(...).Wait()) to “let Phase 2/3 run”. This is prone to flakiness on slower CI machines and can hide real timing issues. Prefer waiting on an observable condition with a timeout (e.g., a specific expected log message via MockLogger.WaitForMessageAsync, or polling Moq invocation count) rather than sleeping for a hardcoded duration.
| // Wait for Phase 2 to run (store.Set called twice — once in Phase 1, once in Phase 2) | |
| Task.Delay(TimeSpan.FromMilliseconds(200)).Wait(); | |
| // Wait for Phase 2 to run by observing the expected update instead of sleeping. | |
| bool phase2Completed = SpinWait.SpinUntil(() => | |
| { | |
| try | |
| { | |
| MockStore.Verify(s => s.Set(AssetName, updatedValue), Times.Once); | |
| return true; | |
| } | |
| catch (MockException) | |
| { | |
| return false; | |
| } | |
| }, Timeout); | |
| phase2Completed.Should().BeTrue("Phase 2 should update the store within the test timeout."); |
There was a problem hiding this comment.
Fixed in 06a8715. All Task.Delay(...).Wait() calls replaced: Phase 2 tests now await MockLogger.WaitForMessageAsync(...) on the terminal log for each code path (AssetUpdated, AssetUpToDate, BackgroundFetchFailed); the Phase 3 DownloadsAndUpdatesStore test uses SpinWait.SpinUntil(() => MockSignal.Invocations.Any(), Timeout) since RequestRecycle is the last observable action in that cycle. Affected test methods were also converted to async Task and given the Async suffix.
Generated by Claude Code
| await Phase2Async(stoppingToken); | ||
| await Phase3Async(stoppingToken); |
There was a problem hiding this comment.
Phase 1 exceptions are handled to avoid BackgroundServiceExceptionBehavior.StopHost, but Phase2Async/Phase3Async are awaited without any outer try/catch. If DeserializeAsync or cache SaveAsync throws during Phase 2/3, the hosted service will fault and can still stop the host unexpectedly. Consider wrapping Phase 2 and Phase 3 in exception handling that logs and continues (or transitions to FatalError intentionally), consistent with the Phase 1 rationale, and make sure cancellation is still honored.
| await Phase2Async(stoppingToken); | |
| await Phase3Async(stoppingToken); | |
| try | |
| { | |
| await Phase2Async(stoppingToken); | |
| await Phase3Async(stoppingToken); | |
| } | |
| catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) | |
| { | |
| return; | |
| } | |
| catch (Exception ex) | |
| { | |
| _log.CloudAssetOrchestrator_Failed(ex); | |
| // Do not re-throw: later-phase failures should not fault the hosted service and | |
| // trigger BackgroundServiceExceptionBehavior.StopHost during normal app execution. | |
| return; | |
| } |
There was a problem hiding this comment.
Fixed in 06a8715. ExecuteAsync now wraps Phase2Async/Phase3Async in a separate try/catch — OperationCanceledException when stoppingToken.IsCancellationRequested for clean shutdown, and a general Exception catch that logs via CloudAssetOrchestrator_Failed and returns, consistent with the Phase 1 rationale.
Generated by Claude Code
jodavis
left a comment
There was a problem hiding this comment.
Generated by Claude Code
| => new(new MockOptions<CloudSettings>(new CloudSettings { CachePath = cachePath }), fileSystem.Object); | ||
|
|
||
| [TestMethod] | ||
| public async Task CloudAssetCache_SaveAsync_WritesFileWithCorrectContentAsync() |
There was a problem hiding this comment.
Don't use 'async Task' test methods. Use TaskCompletionSource instead to simulate async interactions in synchronous tests. That makes it possible to test all the subtle timing differences that are possible in this complicated parallel algorithm.
There was a problem hiding this comment.
Fixed in 1315933. All async Task test methods in CloudAssetCacheTests and the orchestrator Phase 2/3 tests have been converted to void. Background completion is observed via MockLogger.WaitForMessageAsync(...).Should().BeCompleteWithin(Timeout) (giving clear timeout error messages) and SpinWait.SpinUntil where a log message isn't the right termination condition. Task<Stream?> results are read via .Result after the fluent completion assertion.
Generated by Claude Code
| [LoggerMessage(EventId = 1705, Level = LogLevel.Warning, Message = "Failed to download latest '{AssetName}' from server; keeping cached version")] | ||
| public partial void CloudAssetOrchestrator_BackgroundFetchFailed(string assetName); | ||
|
|
||
| [LoggerMessage(EventId = 1706, Level = LogLevel.Information, Message = "Stub file change detected; refreshing asset '{AssetName}'")] |
There was a problem hiding this comment.
I'm not sure what this error message is trying to say. It needs to be clearer to someone who doesn't know the internal implementation.
There was a problem hiding this comment.
Fixed in bb10d82. Changed to "Layout service reported a change; re-downloading asset '{AssetName}'", which describes the observable behaviour (the layout service signalled a change) without exposing that it's backed by a stub file.
| } | ||
| catch | ||
| { | ||
| _log.CloudAssetOrchestrator_BackgroundFetchFailed(asset.Name); |
There was a problem hiding this comment.
Log the exceptions in all error logging messages, so the problem can be diagnosed.
There was a problem hiding this comment.
Fixed in bb10d82. BackgroundFetchFailed now takes Exception? exception. All catch blocks use catch (Exception ex) and pass ex; null-stream code paths pass null (no exception to attach). 2af8462 also adds CloudAssetOrchestrator_UnknownAssetChange to log a warning when Phase 3 receives a notification for an unrecognised asset name.
| return; | ||
| } | ||
|
|
||
| foreach (ICloudAsset asset in _assets) |
There was a problem hiding this comment.
I know we only have one asset for now, but this is going to redownload all the assets every time we receive a notification for one of them?
There was a problem hiding this comment.
Fixed in bb10d82. IAssetChangeNotifier.WaitForChangeAsync now returns Task<string> — the name of the asset that changed. CloudSettings.AssetName (default "layout") is the value FileSystemCloudAssetWatchService returns. Phase 3 now calls _assets.FirstOrDefault(a => a.Name == changedAssetName) and only processes that single asset; unknown names are logged as a warning (2af8462) and skipped.
| object value = await asset.DeserializeAsync(new MemoryStream(serverBytes), ct); | ||
| await _cache.SaveAsync(asset.Name, new MemoryStream(serverBytes), ct); | ||
| _store.Set(asset.Name, value); | ||
| IdleDeferRecycle(); |
There was a problem hiding this comment.
_log.CloudAssetOrchestrator_AssetUpdated() here
There was a problem hiding this comment.
Fixed in bb10d82. _log.CloudAssetOrchestrator_AssetUpdated(asset.Name) is now called inside the consolidated ApplyServerUpdateAsync helper, which Phase 3 shares with Phase 2.
|
|
||
| try | ||
| { | ||
| byte[] serverBytes; |
There was a problem hiding this comment.
Seems like a lot of duplication between this and the end of phase 2, but there's some inconsistency (no logging message, no cache hash check). We should consolidate this into a helper method so they stay consistent.
There was a problem hiding this comment.
Fixed in bb10d82. Phase 2 and Phase 3 both now call ApplyServerUpdateAsync(asset, serverBytes, ct), which performs the hash comparison, deserialization, cache save, store update, AssetUpdated log, and idle-deferred recycle in one place.
| And the stub layout file is set to the updated layout | ||
| When I start the application | ||
| Then I should see the application in the Ready phase | ||
| And I should see the "Guide" button exists |
There was a problem hiding this comment.
Add And I should see the application recycle. Set the idle delay to some non-zero value so that the test has time to catch the recycle. We'll also need some kind of recycle detection mechanism.
There was a problem hiding this comment.
Fixed in bb10d82. The background refresh scenario now includes And the idle cooldown is 2 seconds (which calls ISimulatedEnvironment.SetIdleCooldownSeconds) so the deferred-recycle path is exercised. And I should see the application recycle checks for "Recycling application scope" in the log file with retries. The Given the idle cooldown is {n} seconds step is backed by SetIdleCooldownSeconds(int) on SimulatedEnvironment.
| And the stub layout file is set to the primary layout | ||
| When I start the application | ||
| Then I should see the application in the Ready phase | ||
| And I should not see the Guide button |
There was a problem hiding this comment.
Add some And I should see a message in the logs: steps to ensure that the layout was downloaded as expected.
Do this for all the scenarios. Also add "And I should not see any warnings or errors in the logs" at the end of the scenarios. That will help detect any unexpected failures, since the fallback behavior hides those failures from the user.
There was a problem hiding this comment.
Fixed in bb10d82. All applicable scenarios now have And I should see a message in the logs: assertions for the key phase log messages (download, cache hit, up-to-date, updated, file-change detected) and And I should not see any warnings or errors in the logs at the end. Scenarios with expected warnings/errors (fatal error, background download fail) use the appropriate narrower check.
| And the stub layout file is set to the primary layout | ||
| When I start the application | ||
| Then I should see the application in the Ready phase | ||
| And I should not see the Guide button |
There was a problem hiding this comment.
Here you can also add an And I should not see a message containing 'recycling' in the log to ensure that no update really occurred.
There was a problem hiding this comment.
Fixed in bb10d82. Added And I should not see a message containing 'recycling' in the logs to the "no update" scenario.
| [Given(@"the stub layout file is set to the primary layout")] | ||
| public void GivenTheStubLayoutFileIsSetToThePrimaryLayout() | ||
| { | ||
| File.WriteAllText(StubFilePath, File.ReadAllText(PrimaryLayoutPath)); |
There was a problem hiding this comment.
Fixed in bb10d82. All file copies now use File.Copy(source, dest, overwrite: true) in both the step methods and the WriteToCacheFile helper.
| WriteToCacheFile(UpdatedLayoutPath); | ||
| } | ||
|
|
||
| [Given(@"the stub layout file is set to the primary layout")] |
There was a problem hiding this comment.
Instead of the stub file, refer to the compiled layout service as if the service were completely implemented. Eventually it will be implemented and we can change the steps to simulate the scenario differently, without changing all the tests.
There was a problem hiding this comment.
Fixed in bb10d82. All Gherkin steps and step bindings now say "compiled layout service" — e.g. Given the compiled layout service serves the primary layout, Given the compiled layout service is unavailable, When the compiled layout service is updated to the updated layout. The feature file description also references "compiled layout service" throughout.
|
@copilot Resolve all the code review comments. When you're finished, make sure the entire repo builds with no warnings or errors and that all tests pass. |
3831e42 to
01cfce2
Compare
2af8462 to
0344484
Compare
01cfce2 to
9933634
Compare
…nd refresh, and file-watch loop
Replaces the stub CloudAssetOrchestrator with a three-phase implementation:
- Phase 1: cache-first parallel load — reads from {CachePath}/{name}.cache if present,
otherwise downloads from server and saves to cache; signals IPreScopeInitializer once
all assets are in the store (or faults on failure).
- Phase 2: background server refresh — for cache-loaded assets, compares SHA-256 of
server bytes against cached bytes; updates store/cache and schedules an idle-deferred
scope recycle when content differs.
- Phase 3: file-change loop — waits on IAssetChangeNotifier.WaitForChangeAsync; on each
notification re-downloads all assets, updates cache/store, and schedules a recycle.
New components:
- CloudAssetCache: IFileSystem-backed implementation of ICloudAssetCache
- IAssetChangeNotifier: decoupled notification contract for Phase 3
- FileSystemCloudAssetWatchService: debounced FileSystemWatcher (100 ms cancel-restart)
implementing IAssetChangeNotifier as a singleton BackgroundService
- Log messages 1702-1706 (LoadedFromCache, AssetUpToDate, AssetUpdated,
BackgroundFetchFailed, FileChangeDetected)
Phase 1 failure sets the lifecycle FatalError state via the faulted WaitAsync and returns
cleanly rather than re-throwing, preventing BackgroundServiceExceptionBehavior.StopHost
from killing the process before the FatalError UI state can be observed by tests.
E2E test infrastructure:
- primary-layout.json / updated-layout.json fixtures (Guide absent/present in WELL group)
- Cloud paths configured once per test run in BeforeTestRun; @cloud-layout scenarios
stop the host in AfterScenario to prevent state leakage
- ApplicationTestService resolves IRemoteDefinitionService lazily so GetCurrentPhaseAsync
works in FatalError state where the layout asset is absent
https://claude.ai/code/session_018JQNh2RgmSxxNxzH9FM2o3
…nsistency. - Added a few more logging messages - Changed the output of ICloudAssetChangeNotifier to be an ICloudAsset - Fixed unit tests to expect the new, consistent behavior
a528464 to
175ee57
Compare
56f1f4c
into
feature/ADR-162-client-side-layout-updates
$(cat <<'EOF'
https://jodasoft.atlassian.net/browse/ADR-181
Summary
CloudAssetOrchestratorwith a three-phase real implementation: Phase 1 loads eachICloudAssetfrom a local file cache in parallel and signalsIPreScopeInitializercomplete once all assets are in the store (or faults on failure); Phase 2 background-checks cached assets against the server and schedules an idle-deferred scope recycle when content differs; Phase 3 loops onIAssetChangeNotifier.WaitForChangeAsyncand re-applies updates whenever the stub file changes.CloudAssetCache(file-backedICloudAssetCacheusingIFileSystemfor testability),IAssetChangeNotifier(decoupled notification contract), andFileSystemCloudAssetWatchService(debouncedFileSystemWatchersingleton with cancel-restart pattern).WaitAsyncand returns cleanly rather than re-throwing, soApplicationLifecyclecan set theFatalErrorUI state withoutBackgroundServiceExceptionBehavior.StopHostkilling the process first.@cloud-layouttag withAfterScenariohost teardown.ApplicationTestServicenow resolvesIRemoteDefinitionServicelazily soGetCurrentPhaseAsyncworks inFatalErrorstate where the layout asset is absent from the store.Test plan
dotnet build /warnaserror— zero warnings across App, Tests, and Headless projectsdotnet test test/AdaptiveRemote.App.Tests— all 390 unit tests pass (CloudAssetCacheTests, CloudAssetOrchestratorTests, FileSystemCloudAssetWatchServiceTests included)dotnet test test/AdaptiveRemote.EndToEndTests.Host.Headless— all 21 E2E tests pass, including the 7 new@cloud-layoutscenariosdev/layout.jsonstub configured — app should reach Ready; restart with cache present — app should reach Ready faster (cache hit path)https://claude.ai/code/session_018JQNh2RgmSxxNxzH9FM2o3
EOF
)
Generated by Claude Code