From 971a9886b81f1e342f34d1b74ea3505539272db9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:35:02 -0400 Subject: [PATCH] fix(deployment): drop dead ITemplateGraphWatermark ctor param and no-op ClearMemos from StaleInstanceProbe (review 2026-10-21 04/U2) --- .../StaleInstanceProbe.cs | 13 ++----- .../DeploymentServiceTests.cs | 6 +-- .../StaleInstanceProbeTests.cs | 37 ++++++++++--------- .../Import/BundleImporterApplyTests.cs | 4 -- 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs index 799e113a..c07ff89c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs @@ -1,21 +1,19 @@ -using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; namespace ZB.MOM.WW.ScadaBridge.DeploymentManager; /// Recomputes deployment revisions from authoritative configuration on every probe. -/// Process-local watermarks cannot prove freshness after another central changes the shared store. +/// Process-local watermarks cannot prove freshness after another central changes the shared store, +/// so nothing is memoised across calls. public sealed class StaleInstanceProbe : IStaleInstanceProbe { private readonly IFlatteningPipeline _flatteningPipeline; - /// Initializes the authoritative probe. Watermark remains a constructor compatibility dependency. + /// Initializes the authoritative probe. /// The pipeline used to flatten instances for staleness comparison. - /// The template graph watermark; retained only for constructor compatibility. - public StaleInstanceProbe(IFlatteningPipeline flatteningPipeline, ITemplateGraphWatermark watermark) + public StaleInstanceProbe(IFlatteningPipeline flatteningPipeline) { _flatteningPipeline = flatteningPipeline ?? throw new ArgumentNullException(nameof(flatteningPipeline)); - ArgumentNullException.ThrowIfNull(watermark); } /// @@ -26,7 +24,4 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe validateScripts: false, session).ConfigureAwait(false); return result.IsSuccess ? result.Value.RevisionHash : null; } - - /// Compatibility hook: probes no longer retain process-global results. - public static void ClearMemos() { } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index 617f46df..2839e223 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -717,16 +717,12 @@ public class DeploymentServiceTests : TestKit public async Task StaleInstanceProbe_RequestsValidateScriptsFalse() { // The Transport stale-instance probe also only needs the hash — same skip. - // The probe memoises per-instance against the graph watermark, so start - // from a clean memo or a sibling test's entry would satisfy the probe - // without a flatten at all. - StaleInstanceProbe.ClearMemos(); _pipeline.CreateSession().Returns(_ => new FlattenSession(new TemplateGraphWatermark())); _pipeline.FlattenAndValidateAsync(5, Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(new FlattenedConfiguration(), "sha256:probe", ValidationResult.Success()))); - var probe = new StaleInstanceProbe(_pipeline, new TemplateGraphWatermark()); + var probe = new StaleInstanceProbe(_pipeline); var hash = await probe.GetCurrentRevisionHashAsync(5); diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs index cd93eb51..0bb3b947 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs @@ -8,20 +8,11 @@ using ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening; namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests; /// -/// The staleness fast path: memoises a computed -/// revision hash against the readings it was -/// computed under, and the memo is PROCESS-STATIC. That makes the watermark the -/// only thing standing between a cached hash and a wrong answer — so these tests -/// pin that the memo is served only while the watermark has not moved, and is -/// dropped the moment it has. -/// -/// -/// The failure this guards against is a real one: a data-connection edit, or a -/// bundle import committing through the raw DbContext, changes the flattened -/// output without going through the repository path that derives bumps from the -/// change tracker. If that write does not bump, the memo below keeps answering with -/// the old hash forever. -/// +/// is authoritative: it re-flattens on EVERY probe and +/// memoises nothing across calls. It used to memoise a revision hash against the +/// process-local , which a peer central node writing +/// the shared database could never bump (MG-3) — these tests pin that every probe sees +/// the current configuration whatever the local watermark says. /// public class StaleInstanceProbeTests { @@ -35,9 +26,6 @@ public class StaleInstanceProbeTests /// Initializes a new instance of the StaleInstanceProbeTests class. public StaleInstanceProbeTests() { - // Process-static memo: start every test from a clean slate. - StaleInstanceProbe.ClearMemos(); - _pipeline.CreateSession().Returns(_ => new FlattenSession(_watermark)); _pipeline .FlattenAndValidateAsync(InstanceId, Arg.Any(), Arg.Any(), Arg.Any()) @@ -50,7 +38,20 @@ public class StaleInstanceProbeTests }); } - private StaleInstanceProbe CreateProbe() => new(_pipeline, _watermark); + private StaleInstanceProbe CreateProbe() => new(_pipeline); + + /// + /// review 2026-10-21 04/U2: with the memo gone the probe has no use for the watermark, so + /// its only dependency is the pipeline and the no-op ClearMemos hook is gone. + /// + [Fact] + public void Probe_DependsOnlyOnThePipeline_AndHasNoMemoHook() + { + var ctor = Assert.Single(typeof(StaleInstanceProbe).GetConstructors()); + var parameter = Assert.Single(ctor.GetParameters()); + Assert.Equal(typeof(IFlatteningPipeline), parameter.ParameterType); + Assert.Null(typeof(StaleInstanceProbe).GetMethod("ClearMemos")); + } /// Verifies that peer edit with independent watermark is observed by warm active probe. /// A task that represents the asynchronous operation. diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index 8716b988..fcabceb6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -1595,10 +1595,6 @@ public sealed class BundleImporterApplyTests : IDisposable [Fact] public async Task ApplyAsync_second_overwrite_of_same_template_still_marks_instance_stale() { - // The memo is process-static and keyed by instance id alone; start clean so - // this test does not inherit another class's entry. - StaleInstanceProbe.ClearMemos(); - await SeedSiteAsync(); // Shape A → bundle A.