diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index fc1740e..c529247 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -318,7 +318,7 @@ longer imposes a fixed absolute cap. `dotnet build ScadaLink.slnx` clean; |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor:196-216` | **Description** @@ -341,16 +341,59 @@ If polling is kept as a fallback, fetch only changed/in-progress records. **Resolution** -_Unresolved — a genuine SignalR-push fix requires an event source in another -module._ Verified 2026-05-16: `Deployments.razor` does poll every 10s, contrary -to the design doc. But a real push implementation needs the **Deployment -Manager** module (`ScadaLink.DeploymentManager` — `DeploymentService` / -`ArtifactDeploymentService` write the `DeploymentRecord` rows) to raise a -status-change event/observable that the page subscribes to; there is no such -event today and no CentralUI-only seam to subscribe to. Building that event -source is out of scope for a CentralUI-only review. Left Open and surfaced for a -follow-up that adds a deployment-status broadcaster in DeploymentManager (or a -design-doc amendment acknowledging the polling fallback). +Resolved 2026-05-16 (commit ``) — cross-module fix (CentralUI + +DeploymentManager), explicitly authorized. Root cause confirmed against the +source: `Deployments.razor` ran a `Timer` (`OnInitializedAsync` → `StartTimer`, +10s interval) that, every tick and for every open Blazor circuit, reloaded all +deployment records (`GetAllDeploymentRecordsAsync`) and the full instance map +(`GetAllInstancesAsync`) — contradicting Component-CentralUI "Real-Time Updates" +("transitions push to the UI immediately via SignalR … no polling required"). + +**Process/DI topology confirmed.** `ScadaLink.Host/Program.cs` calls both +`AddDeploymentManager()` (line 75) and `AddCentralUI()` (line 77) on the same +`builder.Services` — DeploymentManager and the Central UI run **in the same +central Host process**, so a DI singleton is genuinely shared between the +DeploymentManager services and the Blazor circuit's scoped components. The +shared-singleton seam is real; no out-of-process fallback was needed. + +**What was implemented — push-based updates.** A new +`IDeploymentStatusNotifier` (`ScadaLink.DeploymentManager/IDeploymentStatusNotifier.cs`) +with a C# `event Action` and a small payload +(`DeploymentStatusChange` = deployment id + instance id + new status). Its +implementation `DeploymentStatusNotifier` invokes each subscriber in isolation +and swallows/logs handler exceptions so a faulting circuit cannot break the +deployment pipeline. It is registered as a **singleton** in `AddDeploymentManager` +(`ServiceCollectionExtensions`). Every place `DeploymentService` writes a +`DeploymentRecord` status now raises the notifier: the `Pending` create, the +`InProgress` update, the site-response terminal update, the `Failed` cleanup +write in the catch block, and the `DeploymentManager-006` reconciled-`Success` +write — five call sites via a private `NotifyStatusChange` helper. +`ArtifactDeploymentService` was inspected and writes only +`SystemArtifactDeploymentRecord` rows, which `Deployments.razor` does not +display, so it correctly raises nothing. `Deployments.razor` no longer has a +`Timer`: `OnInitializedAsync` subscribes to `IDeploymentStatusNotifier.StatusChanged`, +the handler reloads via `InvokeAsync(StateHasChanged)` (the notifier event is +raised on the DeploymentManager service thread), and `Dispose` unsubscribes. +Blazor Server pushes the re-render to the browser over its SignalR circuit +automatically — satisfying the documented design. The existing "Pause/Resume +updates" toggle now gates whether incoming push events are acted on, and +"Refresh" still forces a manual reload. CLAUDE.md UI rules kept: Blazor Server + +Bootstrap, custom components, no third-party frameworks. + +Regression tests fail against the pre-fix code and pass after. DeploymentManager +(`DeploymentStatusNotifierTests`): `DeployInstanceAsync_RaisesStatusChange_ForEveryRecordStatusWrite` +(pre-fix: no notifier, fails to compile / silent), plus +`NotifyStatusChanged_WithNoSubscribers_DoesNotThrow` and +`NotifyStatusChanged_ThrowingSubscriber_DoesNotBreakOtherSubscribers`; +`ServiceCollectionExtensionsTests.AddDeploymentManager_RegistersDeploymentStatusNotifier_AsSingleton` +pins the shared-singleton seam. CentralUI (`DeploymentsPushUpdateTests`): +`Deployments_DoesNotPoll_HasNoRefreshTimer` (pre-fix: the `_refreshTimer` field +existed — confirmed failing), `Deployments_StatusChange_TriggersReload`, and +`Deployments_Dispose_UnsubscribesFromNotifier`. `dotnet build ScadaLink.slnx` +clean (0 warnings); `tests/ScadaLink.DeploymentManager.Tests` 76 passed, +`tests/ScadaLink.CentralUI.Tests` 257 passed. (`TopologyPageTests`' DI fixture +was also updated to register the new notifier, since it constructs the real +`DeploymentService`.) ### CentralUI-007 — Monitoring nav links to Deployment-only pages are shown to all roles diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor index 327f402..bb184af 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor @@ -8,6 +8,7 @@ @inject IDeploymentManagerRepository DeploymentManagerRepository @inject ITemplateEngineRepository TemplateEngineRepository @inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope +@inject ScadaLink.DeploymentManager.IDeploymentStatusNotifier DeploymentStatusNotifier @implements IDisposable
@@ -196,47 +197,42 @@ private Dictionary _instanceNames = new(); private bool _loading = true; private string? _errorMessage; - private Timer? _refreshTimer; private bool _autoRefresh = true; private readonly HashSet _expandedErrors = new(); private int _currentPage = 1; private int _totalPages; private const int PageSize = 25; - private static readonly TimeSpan RefreshInterval = TimeSpan.FromSeconds(10); + + // CentralUI-006: deployment status updates are push-based, not polled. + // DeploymentManager raises IDeploymentStatusNotifier.StatusChanged on every + // deployment-record status write; this page subscribes to it and reloads, + // and Blazor Server pushes the re-render to the browser over its SignalR + // circuit — satisfying the design's "no polling required" requirement. + // The notifier event is raised on the DeploymentManager service thread, so + // the handler marshals onto the renderer via InvokeAsync. protected override async Task OnInitializedAsync() { await LoadDataAsync(); - StartTimer(); + DeploymentStatusNotifier.StatusChanged += OnDeploymentStatusChanged; } - private void StartTimer() + private void OnDeploymentStatusChanged(ScadaLink.DeploymentManager.DeploymentStatusChange change) { - _refreshTimer?.Dispose(); - _refreshTimer = new Timer(_ => + if (!_autoRefresh) return; + _ = InvokeAsync(async () => { - InvokeAsync(async () => - { - if (!_autoRefresh) return; - await LoadDataAsync(); - StateHasChanged(); - }); - }, null, RefreshInterval, RefreshInterval); + await LoadDataAsync(); + StateHasChanged(); + }); } private void ToggleAutoRefresh() { + // When paused, incoming push notifications are ignored; "Refresh" still + // forces a manual reload. No timer is involved either way. _autoRefresh = !_autoRefresh; - if (_autoRefresh) - { - StartTimer(); - } - else - { - _refreshTimer?.Dispose(); - _refreshTimer = null; - } } private bool IsErrorExpanded(string deploymentId) => _expandedErrors.Contains(deploymentId); @@ -320,6 +316,8 @@ public void Dispose() { - _refreshTimer?.Dispose(); + // Unsubscribe so a status change after the circuit is gone does not + // touch a disposed component (the notifier is a process singleton). + DeploymentStatusNotifier.StatusChanged -= OnDeploymentStatusChanged; } } diff --git a/src/ScadaLink.DeploymentManager/DeploymentService.cs b/src/ScadaLink.DeploymentManager/DeploymentService.cs index 46180c9..039b7c4 100644 --- a/src/ScadaLink.DeploymentManager/DeploymentService.cs +++ b/src/ScadaLink.DeploymentManager/DeploymentService.cs @@ -41,6 +41,7 @@ public class DeploymentService private readonly OperationLockManager _lockManager; private readonly IAuditService _auditService; private readonly DiffService _diffService; + private readonly IDeploymentStatusNotifier _statusNotifier; private readonly DeploymentManagerOptions _options; private readonly ILogger _logger; @@ -60,6 +61,7 @@ public class DeploymentService OperationLockManager lockManager, IAuditService auditService, DiffService diffService, + IDeploymentStatusNotifier statusNotifier, IOptions options, ILogger logger) { @@ -70,10 +72,21 @@ public class DeploymentService _lockManager = lockManager; _auditService = auditService; _diffService = diffService; + _statusNotifier = statusNotifier; _options = options.Value; _logger = logger; } + /// + /// CentralUI-006: raises a push notification that a deployment record's + /// status was just persisted, so the Central UI deployment-status page can + /// re-render over its SignalR circuit instead of polling. Called at every + /// point a status is written. + /// + private void NotifyStatusChange(DeploymentRecord record) => + _statusNotifier.NotifyStatusChanged( + new DeploymentStatusChange(record.DeploymentId, record.InstanceId, record.Status)); + /// /// Resolves the site's string identifier from the numeric DB ID. /// The communication layer routes by string identifier (e.g. "site-a"), not DB ID. @@ -155,11 +168,13 @@ public class DeploymentService await _repository.AddDeploymentRecordAsync(record, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); + NotifyStatusChange(record); // Update status to InProgress record.Status = DeploymentStatus.InProgress; await _repository.UpdateDeploymentRecordAsync(record, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); + NotifyStatusChange(record); try { @@ -187,6 +202,7 @@ public class DeploymentService // non-Success record while the site is running the new config. await _repository.UpdateDeploymentRecordAsync(record, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); + NotifyStatusChange(record); if (response.Status == DeploymentStatus.Success) { @@ -253,6 +269,7 @@ public class DeploymentService { await _repository.UpdateDeploymentRecordAsync(record, CancellationToken.None); await _repository.SaveChangesAsync(CancellationToken.None); + NotifyStatusChange(record); await _auditService.LogAsync(user, "DeployFailed", "Instance", instanceId.ToString(), instance.UniqueName, new { DeploymentId = deploymentId, Error = ex.Message }, @@ -624,6 +641,7 @@ public class DeploymentService prior.CompletedAt = DateTimeOffset.UtcNow; await _repository.UpdateDeploymentRecordAsync(prior, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); + NotifyStatusChange(prior); await _auditService.LogAsync(prior.DeployedBy, "DeployReconciled", "Instance", instance.Id.ToString(), instance.UniqueName, diff --git a/src/ScadaLink.DeploymentManager/DeploymentStatusNotifier.cs b/src/ScadaLink.DeploymentManager/DeploymentStatusNotifier.cs new file mode 100644 index 0000000..f5b29a8 --- /dev/null +++ b/src/ScadaLink.DeploymentManager/DeploymentStatusNotifier.cs @@ -0,0 +1,51 @@ +using Microsoft.Extensions.Logging; + +namespace ScadaLink.DeploymentManager; + +/// +/// Default implementation. A simple +/// in-process event broadcaster: registered as a DI singleton so it is shared +/// between the central-process and the Central +/// UI's Blazor circuits (CentralUI-006). +/// +/// A throwing subscriber must never break the deployment pipeline, so each +/// handler is invoked individually and its exceptions are caught and logged. +/// +public sealed class DeploymentStatusNotifier : IDeploymentStatusNotifier +{ + private readonly ILogger _logger; + + public DeploymentStatusNotifier(ILogger logger) + { + _logger = logger; + } + + /// + public event Action? StatusChanged; + + /// + public void NotifyStatusChanged(DeploymentStatusChange change) + { + var handlers = StatusChanged; + if (handlers == null) + return; + + // Invoke each subscriber in isolation: one faulting handler (e.g. a + // disposed Blazor circuit) must not stop the others from being notified + // and must not propagate back into the deployment pipeline. + foreach (var handler in handlers.GetInvocationList()) + { + try + { + ((Action)handler)(change); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "A deployment-status-change subscriber threw for deployment {DeploymentId} " + + "(status {Status}); continuing with remaining subscribers", + change.DeploymentId, change.Status); + } + } + } +} diff --git a/src/ScadaLink.DeploymentManager/IDeploymentStatusNotifier.cs b/src/ScadaLink.DeploymentManager/IDeploymentStatusNotifier.cs new file mode 100644 index 0000000..aa43ef4 --- /dev/null +++ b/src/ScadaLink.DeploymentManager/IDeploymentStatusNotifier.cs @@ -0,0 +1,49 @@ +using ScadaLink.Commons.Types.Enums; + +namespace ScadaLink.DeploymentManager; + +/// +/// Payload describing a single deployment-record status change. Kept small — +/// just the deployment identity, the owning instance, and the new status — so +/// it is cheap to raise on the hot path and cheap for subscribers to handle. +/// +/// The unique deployment ID whose status changed. +/// The instance the deployment record belongs to. +/// The status the deployment record was just written with. +public readonly record struct DeploymentStatusChange( + string DeploymentId, + int InstanceId, + DeploymentStatus Status); + +/// +/// CentralUI-006: push-based deployment-status change notification. +/// +/// The design (Component-CentralUI "Real-Time Updates") requires deployment +/// status transitions to push to the UI immediately via SignalR, with no +/// polling. raises +/// whenever it writes a +/// status; the Central UI's deployment-status page subscribes to it and +/// re-renders over its existing Blazor Server SignalR circuit. +/// +/// Registered as a DI singleton (see ) +/// so the scoped and the Blazor circuit's +/// scoped page component share the same instance — both run in the same +/// central Host process. +/// +public interface IDeploymentStatusNotifier +{ + /// + /// Raised after a deployment record's status has been written. Handlers run + /// synchronously on the caller's thread; subscribers must not block and + /// should marshal any UI work onto their own dispatcher. + /// + event Action? StatusChanged; + + /// + /// Raises . Called by + /// at every point a deployment record's status is persisted. A throwing + /// subscriber must not break the deployment pipeline, so handler exceptions + /// are swallowed by the implementation. + /// + void NotifyStatusChanged(DeploymentStatusChange change); +} diff --git a/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs b/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs index 7efeeb2..f26745d 100644 --- a/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs @@ -27,6 +27,14 @@ public static class ServiceCollectionExtensions // the declared option-class defaults apply. services.AddOptions(); services.AddSingleton(); + + // CentralUI-006: push-based deployment-status notification. Registered + // as a singleton so the scoped DeploymentService and the Central UI's + // scoped Blazor page component share one instance — both run in the + // same central Host process. The deployment-status page subscribes to + // it instead of polling the database every 10 seconds. + services.AddSingleton(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/tests/ScadaLink.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs b/tests/ScadaLink.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs new file mode 100644 index 0000000..612af87 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs @@ -0,0 +1,112 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ScadaLink.CentralUI.Auth; +using ScadaLink.Commons.Entities.Deployment; +using ScadaLink.Commons.Entities.Instances; +using ScadaLink.Commons.Interfaces.Repositories; +using ScadaLink.Commons.Types.Enums; +using ScadaLink.DeploymentManager; +using DeploymentsPage = ScadaLink.CentralUI.Components.Pages.Deployment.Deployments; + +namespace ScadaLink.CentralUI.Tests.Deployment; + +/// +/// Regression tests for CentralUI-006. Component-CentralUI "Real-Time Updates" +/// states deployment status transitions push to the UI immediately via SignalR +/// with no polling. The page previously ran a 10-second Timer that +/// reloaded every deployment record + instance map per tick. The fix removes +/// the timer and subscribes to , which +/// DeploymentService raises on every deployment-record status write; +/// Blazor Server then pushes the re-render over its SignalR circuit. +/// +public class DeploymentsPushUpdateTests : BunitContext +{ + private IDeploymentManagerRepository _deployRepo = null!; + private ITemplateEngineRepository _templateRepo = null!; + private DeploymentStatusNotifier _notifier = null!; + + private void RegisterServices() + { + _deployRepo = Substitute.For(); + _templateRepo = Substitute.For(); + _notifier = new DeploymentStatusNotifier(NullLogger.Instance); + + _templateRepo.GetAllInstancesAsync(Arg.Any()) + .Returns(new List + { + new("Inst-1") { Id = 1, SiteId = 1 } + }); + _deployRepo.GetAllDeploymentRecordsAsync(Arg.Any()) + .Returns(new List()); + + Services.AddSingleton(_deployRepo); + Services.AddSingleton(_templateRepo); + Services.AddSingleton(_notifier); + + var identity = new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Name, "deployer") }, "TestCookie"); + var stubAuth = new StubAuthStateProvider( + new AuthenticationState(new ClaimsPrincipal(identity))); + Services.AddSingleton(stubAuth); + Services.AddScoped(_ => new SiteScopeService(stubAuth)); + } + + private sealed class StubAuthStateProvider : AuthenticationStateProvider + { + private readonly AuthenticationState _state; + public StubAuthStateProvider(AuthenticationState state) => _state = state; + public override Task GetAuthenticationStateAsync() + => Task.FromResult(_state); + } + + [Fact] + public void Deployments_DoesNotPoll_HasNoRefreshTimer() + { + // The 10-second polling Timer must be gone — push replaces polling. + var timerField = typeof(DeploymentsPage).GetField( + "_refreshTimer", BindingFlags.Instance | BindingFlags.NonPublic); + + Assert.Null(timerField); + } + + [Fact] + public void Deployments_StatusChange_TriggersReload() + { + RegisterServices(); + var cut = Render(); + + // Initial load: instances + records each fetched once. + _deployRepo.ClearReceivedCalls(); + _templateRepo.ClearReceivedCalls(); + + // A deployment status write in DeploymentManager raises the notifier; + // the page must reload in response (no polling timer involved). + _notifier.NotifyStatusChanged( + new DeploymentStatusChange("dep-1", 1, DeploymentStatus.Success)); + + cut.WaitForAssertion(() => + _deployRepo.Received().GetAllDeploymentRecordsAsync(Arg.Any())); + } + + [Fact] + public void Deployments_Dispose_UnsubscribesFromNotifier() + { + RegisterServices(); + var cut = Render(); + + cut.Instance.Dispose(); + _deployRepo.ClearReceivedCalls(); + + // After disposal, a status change must NOT touch the disposed component. + _notifier.NotifyStatusChanged( + new DeploymentStatusChange("dep-2", 1, DeploymentStatus.Failed)); + + _deployRepo.DidNotReceive() + .GetAllDeploymentRecordsAsync(Arg.Any()); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs b/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs index 643700c..2397f70 100644 --- a/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs @@ -57,6 +57,11 @@ public class TopologyPageTests : BunitContext // DeploymentService gained a DiffService dependency (DeploymentManager // contract change); register it so the page's DI graph resolves. Services.AddScoped(); + // CentralUI-006: DeploymentService now also depends on the + // deployment-status notifier (a process singleton in production). + Services.AddSingleton( + new ScadaLink.DeploymentManager.DeploymentStatusNotifier( + NullLogger.Instance)); Services.AddScoped(); Services.AddScoped(); Services.AddScoped(); diff --git a/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs index 4276b3d..2931bec 100644 --- a/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -47,7 +47,9 @@ public class DeploymentServiceTests : TestKit var siteRepo = Substitute.For(); _service = new DeploymentService( _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, - new DiffService(), options, + new DiffService(), + new DeploymentStatusNotifier(NullLogger.Instance), + options, NullLogger.Instance); } @@ -577,6 +579,7 @@ public class DeploymentServiceTests : TestKit return new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), NullLogger.Instance); } @@ -792,6 +795,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5), diff --git a/tests/ScadaLink.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs b/tests/ScadaLink.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs new file mode 100644 index 0000000..8049fc1 --- /dev/null +++ b/tests/ScadaLink.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs @@ -0,0 +1,114 @@ +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ScadaLink.Commons.Entities.Deployment; +using ScadaLink.Commons.Entities.Instances; +using ScadaLink.Commons.Interfaces.Repositories; +using ScadaLink.Commons.Interfaces.Services; +using ScadaLink.Commons.Types; +using ScadaLink.Commons.Types.Enums; +using ScadaLink.Commons.Types.Flattening; +using ScadaLink.Communication; +using ScadaLink.TemplateEngine.Flattening; + +namespace ScadaLink.DeploymentManager.Tests; + +/// +/// CentralUI-006: regression tests proving +/// raises whenever it +/// writes a deployment record's status. This is the event source the Central +/// UI deployment-status page subscribes to instead of polling. +/// +public class DeploymentStatusNotifierTests : TestKit +{ + private readonly IDeploymentManagerRepository _repo; + private readonly IFlatteningPipeline _pipeline; + private readonly CommunicationService _comms; + private readonly OperationLockManager _lockManager; + private readonly IAuditService _audit; + private readonly DeploymentStatusNotifier _notifier; + private readonly DeploymentService _service; + + public DeploymentStatusNotifierTests() + { + _repo = Substitute.For(); + _pipeline = Substitute.For(); + _comms = new CommunicationService( + Options.Create(new CommunicationOptions()), + NullLogger.Instance); + _lockManager = new OperationLockManager(); + _audit = Substitute.For(); + _notifier = new DeploymentStatusNotifier(NullLogger.Instance); + + var options = Options.Create(new DeploymentManagerOptions + { + OperationLockTimeout = TimeSpan.FromSeconds(5) + }); + + var siteRepo = Substitute.For(); + _service = new DeploymentService( + _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, + new DiffService(), _notifier, options, + NullLogger.Instance); + } + + [Fact] + public async Task DeployInstanceAsync_RaisesStatusChange_ForEveryRecordStatusWrite() + { + var instance = new Instance("TestInst") { Id = 7, SiteId = 1, State = InstanceState.NotDeployed }; + _repo.GetInstanceByIdAsync(7).Returns(instance); + + var config = new FlattenedConfiguration { InstanceUniqueName = "TestInst" }; + _pipeline.FlattenAndValidateAsync(7, Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(config, "sha256:abc", ValidationResult.Success()))); + + var changes = new List(); + _notifier.StatusChanged += c => changes.Add(c); + + // _comms has no actor set, so the deploy reaches the catch block and + // the record ends Failed. The notifier must fire for the Pending, + // InProgress and Failed writes — not be silent (the pre-fix behaviour). + var result = await _service.DeployInstanceAsync(7, "admin"); + + Assert.True(result.IsFailure); + Assert.NotEmpty(changes); + Assert.All(changes, c => Assert.Equal(7, c.InstanceId)); + Assert.Contains(changes, c => c.Status == DeploymentStatus.Pending); + Assert.Contains(changes, c => c.Status == DeploymentStatus.InProgress); + Assert.Contains(changes, c => c.Status == DeploymentStatus.Failed); + + // All notifications carry the same deployment id (the one created here). + var deploymentId = changes[0].DeploymentId; + Assert.False(string.IsNullOrEmpty(deploymentId)); + Assert.All(changes, c => Assert.Equal(deploymentId, c.DeploymentId)); + } + + [Fact] + public void NotifyStatusChanged_WithNoSubscribers_DoesNotThrow() + { + var notifier = new DeploymentStatusNotifier(NullLogger.Instance); + + var ex = Record.Exception(() => + notifier.NotifyStatusChanged(new DeploymentStatusChange("dep-1", 1, DeploymentStatus.Success))); + + Assert.Null(ex); + } + + [Fact] + public void NotifyStatusChanged_ThrowingSubscriber_DoesNotBreakOtherSubscribers() + { + var notifier = new DeploymentStatusNotifier(NullLogger.Instance); + var reached = false; + + notifier.StatusChanged += _ => throw new InvalidOperationException("boom"); + notifier.StatusChanged += _ => reached = true; + + var ex = Record.Exception(() => + notifier.NotifyStatusChanged(new DeploymentStatusChange("dep-2", 2, DeploymentStatus.InProgress))); + + Assert.Null(ex); + Assert.True(reached, "a faulting subscriber must not stop later subscribers from being notified"); + } +} diff --git a/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs b/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs index e35ce4b..6c0bf17 100644 --- a/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs +++ b/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs @@ -56,4 +56,25 @@ public class ServiceCollectionExtensionsTests { Assert.Equal("ScadaLink:DeploymentManager", ServiceCollectionExtensions.OptionsSection); } + + // CentralUI-006: the deployment-status notifier must be a singleton so the + // scoped DeploymentService and the Central UI's scoped Blazor page share + // one instance — without that, a push notification raised by the service + // would never reach the page's subscription. + [Fact] + public void AddDeploymentManager_RegistersDeploymentStatusNotifier_AsSingleton() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDeploymentManager(); + + using var provider = services.BuildServiceProvider(); + + var fromRoot = provider.GetRequiredService(); + using var scope = provider.CreateScope(); + var fromScope = scope.ServiceProvider.GetRequiredService(); + + Assert.IsType(fromRoot); + Assert.Same(fromRoot, fromScope); + } }