From d962c77bb7fbbd42188c3cdabf568a55b51fc251 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 8 Jul 2026 16:27:38 -0400 Subject: [PATCH] fix(health): evict deleted sites from the aggregator on the periodic site refresh --- .../Actors/CentralCommunicationActor.cs | 17 ++++++++-- .../CentralHealthAggregator.cs | 14 ++++++++ .../ICentralHealthAggregator.cs | 12 +++++++ ...lCommunicationActorClientLifecycleTests.cs | 33 ++++++++++++++++++- .../CentralHealthAggregatorTests.cs | 21 ++++++++++++ .../CentralHealthReportLoopTests.cs | 2 ++ .../Kpi/SiteHealthKpiSampleSourceTests.cs | 3 ++ 7 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/Actors/CentralCommunicationActor.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/Actors/CentralCommunicationActor.cs index bac84c8b..f2d4b207 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/Actors/CentralCommunicationActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/Actors/CentralCommunicationActor.cs @@ -499,7 +499,12 @@ public class CentralCommunicationActor : ReceiveActor var frozen = contacts.ToDictionary( kvp => kvp.Key, kvp => (IReadOnlyList)kvp.Value.AsReadOnly()); - return new SiteAddressCacheLoaded(frozen); + + // Carry the full configured-site identifier set (not just the + // address-bearing subset in `frozen`) so the aggregator prunes only + // genuinely-deleted sites and never an addressless-but-configured one. + var knownSiteIds = sites.Select(s => s.SiteIdentifier).ToList(); + return new SiteAddressCacheLoaded(frozen, knownSiteIds); }).PipeTo(self); } @@ -570,6 +575,12 @@ public class CentralCommunicationActor : ReceiveActor } _log.Info("Site ClusterClient cache refreshed with {0} site(s)", _siteClients.Count); + + // Self-healing eviction: a site deleted from configuration would otherwise + // linger in the aggregator as a permanently-offline tile (and live KPI + // sample source) forever. Prune on every refresh so it disappears within + // one refresh interval without needing a dedicated deletion event. + _serviceProvider.GetService()?.PruneUnknownSites(msg.KnownSiteIds); } // TrackMessageForCleanup removed — the dicts it fed @@ -650,7 +661,9 @@ public record RefreshSiteAddresses; /// discipline. The producer wraps the constructed buckets with /// List<T>.AsReadOnly() before piping to Self. /// -internal record SiteAddressCacheLoaded(IReadOnlyDictionary> SiteContacts); +internal record SiteAddressCacheLoaded( + IReadOnlyDictionary> SiteContacts, + IReadOnlyCollection KnownSiteIds); /// /// Notification sent to debug view subscribers when the stream is terminated diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs index 3f5a2da1..3d568ffb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs @@ -196,6 +196,20 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat return state; } + /// + public void PruneUnknownSites(IReadOnlyCollection knownSiteIds) + { + var known = new HashSet(knownSiteIds, StringComparer.Ordinal); + foreach (var siteId in _siteStates.Keys) + { + if (siteId == CentralHealthReportLoop.CentralSiteId || known.Contains(siteId)) + continue; + if (_siteStates.TryRemove(siteId, out _)) + _logger.LogInformation( + "Site {SiteId} evicted from health aggregator (no longer configured)", siteId); + } + } + /// protected override async Task ExecuteAsync(CancellationToken stoppingToken) { diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ICentralHealthAggregator.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ICentralHealthAggregator.cs index 81cb5104..543c48d5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ICentralHealthAggregator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ICentralHealthAggregator.cs @@ -39,4 +39,16 @@ public interface ICentralHealthAggregator /// The string identifier of the site to look up. /// The for the site, or null if not yet seen. SiteHealthState? GetSiteState(string siteId); + + /// + /// Removes tracked sites not in (i.e. deleted + /// from configuration). The synthetic central id is always retained. + /// Self-healing: called from the CentralCommunicationActor's periodic site + /// refresh, so a deleted site disappears within one refresh interval without + /// needing a dedicated deletion event. Without this the in-memory state only + /// ever grew, leaving a deleted site as a permanently-offline dashboard tile + /// (and a live KPI sample source) forever. + /// + /// The identifiers of all currently-configured sites. + void PruneUnknownSites(IReadOnlyCollection knownSiteIds); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CentralCommunicationActorClientLifecycleTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CentralCommunicationActorClientLifecycleTests.cs index ce421f1a..cb208fb5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CentralCommunicationActorClientLifecycleTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CentralCommunicationActorClientLifecycleTests.cs @@ -10,6 +10,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Communication; using ZB.MOM.WW.ScadaBridge.Communication.Actors; +using ZB.MOM.WW.ScadaBridge.HealthMonitoring; namespace ZB.MOM.WW.ScadaBridge.Communication.Tests; @@ -37,7 +38,37 @@ public class CentralCommunicationActorClientLifecycleTests : TestKit private static SiteAddressCacheLoaded Load(string siteId, params string[] addrs) => new(new Dictionary> - { [siteId] = addrs.ToList().AsReadOnly() }); + { [siteId] = addrs.ToList().AsReadOnly() }, + new[] { siteId }); + + [Fact] + public void PeriodicRefresh_PrunesDeletedSites_FromHealthAggregator() + { + // PLAN-01 Task 18: the periodic site refresh feeds the currently-configured + // site ids to the aggregator so a deleted site is evicted within one + // refresh interval. Repo returns only "site-a". + var siteRepo = Substitute.For(); + siteRepo.GetAllSitesAsync(Arg.Any()) + .Returns(new List { new("Site A", "site-a") }); + var aggregator = Substitute.For(); + var services = new ServiceCollection(); + services.AddScoped(_ => siteRepo); + services.AddSingleton(aggregator); + var provider = services.BuildServiceProvider(); + + var actor = Sys.ActorOf(Props.Create(() => new CentralCommunicationActor( + provider, new DefaultSiteClientFactory(), null))); + + // Trigger the refresh (also fires at PreStart, but drive it explicitly so + // the assertion is deterministic). The load runs on a detached task and + // pipes SiteAddressCacheLoaded back to the actor, which then prunes. + actor.Tell(new RefreshSiteAddresses()); + + AwaitAssert( + () => aggregator.Received().PruneUnknownSites( + Arg.Is>(ids => ids.Contains("site-a"))), + TimeSpan.FromSeconds(3)); + } [Fact] public void AddressEdit_RecreatesClient_WithoutRestartLoop() diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs index c2ca7aeb..bc340ef6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs @@ -473,6 +473,27 @@ public class CentralHealthAggregatorTests Assert.False(aggregator.GetSiteState("site-a")!.IsMetricsStale); } + /// + /// Review 01 [Low]: _siteStates only ever grew — a site deleted from + /// configuration remained a permanently-offline dashboard tile (and a KPI + /// sample source) forever. Pruning against the known-site set evicts it; the + /// synthetic central id is always retained. + /// + [Fact] + public void PruneUnknownSites_RemovesDeletedSites_KeepsKnownAndCentral() + { + var (aggregator, _) = NewAggregator(); + aggregator.ProcessReport(MakeReport("site-a", 1)); + aggregator.ProcessReport(MakeReport("site-b", 1)); + aggregator.ProcessReport(MakeReport(CentralHealthReportLoop.CentralSiteId, 1)); + + aggregator.PruneUnknownSites(new[] { "site-a" }); + + Assert.NotNull(aggregator.GetSiteState("site-a")); + Assert.Null(aggregator.GetSiteState("site-b")); + Assert.NotNull(aggregator.GetSiteState(CentralHealthReportLoop.CentralSiteId)); // synthetic id always kept + } + /// /// Review 01 underdeveloped #7: "when did the site drop" was unanswerable. /// Every online↔offline flip stamps . diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthReportLoopTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthReportLoopTests.cs index 9d3cae53..38ac5966 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthReportLoopTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthReportLoopTests.cs @@ -27,6 +27,7 @@ public class CentralHealthReportLoopTests public IReadOnlyDictionary GetAllSiteStates() => new Dictionary(); public SiteHealthState? GetSiteState(string siteId) => null; + public void PruneUnknownSites(IReadOnlyCollection knownSiteIds) { } } /// @@ -256,6 +257,7 @@ public class CentralHealthReportLoopTests public IReadOnlyDictionary GetAllSiteStates() => new Dictionary(); public SiteHealthState? GetSiteState(string siteId) => null; + public void PruneUnknownSites(IReadOnlyCollection knownSiteIds) { } } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/Kpi/SiteHealthKpiSampleSourceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/Kpi/SiteHealthKpiSampleSourceTests.cs index 5d2ff2a9..dc8f4176 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/Kpi/SiteHealthKpiSampleSourceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/Kpi/SiteHealthKpiSampleSourceTests.cs @@ -166,5 +166,8 @@ public class SiteHealthKpiSampleSourceTests public void MarkHeartbeat(string siteId, DateTimeOffset receivedAt) => throw new NotSupportedException(); + + public void PruneUnknownSites(IReadOnlyCollection knownSiteIds) => + throw new NotSupportedException(); } }