From 65b3c23239f6a971598d3e23b8a5a5c74d773614 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 16:15:24 -0400 Subject: [PATCH] perf(siteruntime): projected single-row deployed-config reads (P1-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HandleEnsureInstanceStarted, HandleDeploymentStateQuery, and HandleEnable each called GetAllDeployedConfigsAsync — materializing every deployed config, including every ConfigJson blob — just to FirstOrDefault one row. A volume-restore reconcile healing hundreds of instances turned that into hundreds of full-table reads. Add SiteStorageService.GetDeployedConfigAsync(name) (single-row, full projection incl. ConfigJson, for callers that (re)start an Instance Actor) and GetDeployedConfigMetadataAsync(name) (single-row, ConfigJson-free projection, for callers that only need DeploymentId/RevisionHash/IsEnabled). Switch the three call sites; leave decisions/semantics identical. HandleEnsureInstanceStarted gets its own _deployedConfigLoader seam, deliberately separate from the existing _configLoader (still used by the boot-time load, which legitimately needs every row) so a test overriding one cannot accidentally gate the other. Generation fencing (StoreGenerationAtRead, Medium wave T1) is untouched: the generation is still captured on the actor thread before the read dispatches. Tests: new SiteStorageServiceTests lock in that both projections return the same values GetAllDeployedConfigsAsync().FirstOrDefault(...) did. The three row-101 ensure-started race tests (ReconcileStartsInstanceTests) are ported to the new seam and still pass, proving generation fencing survives the projection. Proof of failure: since this is a pure I/O-cost fix with no observable behavior change, the new/changed tests were confirmed to fail to even compile against the unfixed code (stash/unstash) — GetDeployedConfigAsync, GetDeployedConfigMetadataAsync, and the deployedConfigLoader ctor param don't exist there. --- .../Actors/DeploymentManagerActor.cs | 47 ++++++--- .../Persistence/SiteStorageService.cs | 99 +++++++++++++++++++ .../Actors/ReconcileStartsInstanceTests.cs | 31 +++--- .../Persistence/SiteStorageServiceTests.cs | 74 ++++++++++++++ 4 files changed, 227 insertions(+), 24 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 54b68218..21bc0fd7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -82,6 +82,21 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private readonly Func>> _configLoader; /// + /// Reads a single deployed config row by instance name. Defaults to + /// ; overridable via the ctor so a + /// test can hold the read open mid-flight (row 101's ensure-started race tests). + /// + /// + /// Register P1-4: the row-67 ensure-started heal used to read through + /// — the same full-table-including-ConfigJson load the boot + /// sequence uses — just to FirstOrDefault one instance. A volume-restore reconcile + /// that raises hundreds of s in a row turned that + /// into hundreds of full-table reads. This seam is deliberately separate from + /// : the two are read at different cardinalities (all rows for + /// boot, one row per heal) and a test overriding one must not accidentally gate the other. + /// + private readonly Func> _deployedConfigLoader; + /// /// Reads the durable data_connection_definitions rows (deferred register rows 89 + 90). /// Defaults to ; /// overridable via the ctor so a test can drive a read failure. @@ -276,6 +291,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// defaults to reading from . Primarily for tests that need to /// drive a read failure. /// + /// + /// Register P1-4: optional override for the single-row deployed-config read the row-67 + /// ensure-started heal uses; defaults to reading from . Primarily + /// for tests that need to hold the read open mid-flight to drive the row-101 race. + /// public DeploymentManagerActor( SiteStorageService storage, ScriptCompilationService compilationService, @@ -292,7 +312,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers Func>>? configLoader = null, ExternalSystemDefinitionCache? externalSystemCache = null, ScriptExecutionScheduler? scriptScheduler = null, - Func>>? connectionDefinitionLoader = null) + Func>>? connectionDefinitionLoader = null, + Func>? deployedConfigLoader = null) { _scriptScheduler = scriptScheduler; _storage = storage; @@ -311,6 +332,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers ? TimeSpan.FromSeconds(options.InstanceSweepIntervalSeconds) : TimeSpan.Zero; _configLoader = configLoader ?? (() => _storage.GetAllDeployedConfigsAsync()); + _deployedConfigLoader = deployedConfigLoader ?? (name => _storage.GetDeployedConfigAsync(name)); _connectionDefinitionLoader = connectionDefinitionLoader ?? (() => _storage.GetAllDataConnectionDefinitionsAsync()); _logger = logger; @@ -1502,8 +1524,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers Task.Run(async () => { await _storage.SetInstanceEnabledAsync(instanceName, true); - var configs = await _storage.GetAllDeployedConfigsAsync(); - var config = configs.FirstOrDefault(c => c.InstanceUniqueName == instanceName); + // Row P1-4: a single-row read — this used to materialize every deployed config + // (including every ConfigJson blob) just to FirstOrDefault this one instance. + var config = await _storage.GetDeployedConfigAsync(instanceName); return new EnableResult(command, config, null, sender); }).ContinueWith(t => { @@ -1595,12 +1618,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers var generationAtRead = _storeWriteGeneration; Task.Run(async () => { - // Read through the same seam the boot load and the sweep read through - // (_configLoader defaults to _storage.GetAllDeployedConfigsAsync, so this is the - // identical production call), which lets a test hold the read open and land the - // result behind a committed disable/delete. - var configs = await _configLoader(); - var config = configs.FirstOrDefault(c => c.InstanceUniqueName == instanceName); + // Register P1-4: read a single row through _deployedConfigLoader (defaults to + // _storage.GetDeployedConfigAsync — a WHERE instance_unique_name = @name read, not + // the full-table-including-ConfigJson load the boot sequence uses), which still lets + // a test hold the read open and land the result behind a committed disable/delete. + var config = await _deployedConfigLoader(instanceName); return new EnsureInstanceStartedResult(command, config, null, generationAtRead); }).ContinueWith(t => t.IsCompletedSuccessfully ? t.Result @@ -2095,7 +2117,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers var sender = Sender; var instanceName = request.InstanceUniqueName; - _storage.GetAllDeployedConfigsAsync().ContinueWith(t => + // Row P1-4: a single-row, ConfigJson-free projection — this used to materialize every + // deployed config (including every ConfigJson blob) just to FirstOrDefault this one + // instance's DeploymentId/RevisionHash. Neither is ever read here. + _storage.GetDeployedConfigMetadataAsync(instanceName).ContinueWith(t => { if (!t.IsCompletedSuccessfully) { @@ -2110,7 +2135,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers request.CorrelationId, instanceName, false, null, null, DateTimeOffset.UtcNow); } - var config = t.Result.FirstOrDefault(c => c.InstanceUniqueName == instanceName); + var config = t.Result; return config == null ? new DeploymentStateQueryResponse( request.CorrelationId, instanceName, false, null, null, DateTimeOffset.UtcNow) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs index 9e7325ca..233f73f3 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs @@ -108,6 +108,89 @@ public class SiteStorageService return results; } + /// + /// Returns the single deployed instance configuration for , + /// or if no row exists for it. + /// + /// + /// Register P1-4: plus a FirstOrDefault was + /// materializing every deployed config — including every ConfigJson blob — just to find + /// one row. A volume-restore reconcile healing hundreds of instances turned that into hundreds + /// of full-table reads. Callers that need the full row (including ConfigJson) for exactly + /// one instance — e.g. to (re)start its Instance Actor — should use this instead. Callers that + /// only need identity/staleness metadata should use , + /// which never touches the config_json column. + /// + /// The unique name of the instance. + /// + /// A task that resolves to the deployed configuration row, or if none + /// exists for the instance. + /// + public async Task GetDeployedConfigAsync(string instanceUniqueName) + { + await using var connection = OpenConnection(); + + await using var command = connection.CreateCommand(); + command.CommandText = @" + SELECT instance_unique_name, config_json, deployment_id, revision_hash, is_enabled, deployed_at + FROM deployed_configurations + WHERE instance_unique_name = @name + LIMIT 1"; + command.Parameters.AddWithValue("@name", instanceUniqueName); + + await using var reader = await command.ExecuteReaderAsync(); + if (!await reader.ReadAsync()) + return null; + + return new DeployedInstance + { + InstanceUniqueName = reader.GetString(0), + ConfigJson = reader.GetString(1), + DeploymentId = reader.GetString(2), + RevisionHash = reader.GetString(3), + IsEnabled = reader.GetInt64(4) != 0, + DeployedAt = reader.GetString(5) + }; + } + + /// + /// Returns the deployment-identity metadata for — + /// (name, deployment id, revision hash, is_enabled) — without reading the + /// config_json column, or if no row exists for it. + /// + /// + /// Register P1-4: for a caller that only needs staleness/identity metadata (e.g. the + /// central deployment-state query, which reports only DeploymentId/RevisionHash), + /// reading the config_json blob is pure waste. This projection is that lighter read. + /// + /// The unique name of the instance. + /// + /// A task that resolves to the metadata row, or if none exists for the + /// instance. + /// + public async Task GetDeployedConfigMetadataAsync(string instanceUniqueName) + { + await using var connection = OpenConnection(); + + await using var command = connection.CreateCommand(); + command.CommandText = @" + SELECT instance_unique_name, deployment_id, revision_hash, is_enabled + FROM deployed_configurations + WHERE instance_unique_name = @name + LIMIT 1"; + command.Parameters.AddWithValue("@name", instanceUniqueName); + + await using var reader = await command.ExecuteReaderAsync(); + if (!await reader.ReadAsync()) + return null; + + return new DeployedConfigMetadata( + InstanceUniqueName: reader.GetString(0), + DeploymentId: reader.GetString(1), + RevisionHash: reader.GetString(2), + IsEnabled: reader.GetInt64(3) != 0); + } + /// /// Stores or updates a deployed instance configuration. Uses UPSERT semantics. /// @@ -939,6 +1022,22 @@ public class DeployedInstance public string DeployedAt { get; init; } = string.Empty; } +/// +/// Deployment-identity metadata for one deployed instance — the projection of +/// that omits ConfigJson. Returned by +/// for callers that only need to +/// answer "is this instance deployed, and at what revision" (register P1-4). +/// +/// The unique name of the instance. +/// The unique deployment identifier. +/// The configuration revision hash for staleness detection. +/// Whether the instance is enabled. +public record DeployedConfigMetadata( + string InstanceUniqueName, + string DeploymentId, + string RevisionHash, + bool IsEnabled); + /// /// Represents a shared script stored locally in SQLite. /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs index a15813bb..e8755edd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ReconcileStartsInstanceTests.cs @@ -92,7 +92,8 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable /// private IActorRef CreateDeploymentManager( IServiceProvider? serviceProvider = null, - Func>>? configLoader = null) + Func>>? configLoader = null, + Func>? deployedConfigLoader = null) { var health = new DeploymentManagerStartupProbe(); var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( @@ -109,7 +110,11 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable configFetcher: null, // In position: an expression tree rejects a named argument that skips a parameter. startupLoadRetryInterval: null, - configLoader: configLoader))); + configLoader: configLoader, + externalSystemCache: null, + scriptScheduler: null, + connectionDefinitionLoader: null, + deployedConfigLoader: deployedConfigLoader))); AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), TimeSpan.FromMilliseconds(25)); @@ -409,7 +414,7 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable { const string instanceName = "RacedDisablePump"; var loader = new GatedConfigLoader(_storage); - var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + var dm = CreateDeploymentManager(deployedConfigLoader: loader.LoadAsync); await _storage.StoreDeployedConfigAsync( instanceName, MakeConfigJson(instanceName), "dep-rd", "rev-rd", true); @@ -451,7 +456,7 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable { const string instanceName = "RacedDeletePump"; var loader = new GatedConfigLoader(_storage); - var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + var dm = CreateDeploymentManager(deployedConfigLoader: loader.LoadAsync); await _storage.StoreDeployedConfigAsync( instanceName, MakeConfigJson(instanceName), "dep-rx", "rev-rx", true); @@ -487,7 +492,7 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable { const string instanceName = "UnracedPump"; var loader = new GatedConfigLoader(_storage); - var dm = CreateDeploymentManager(configLoader: loader.LoadAsync); + var dm = CreateDeploymentManager(deployedConfigLoader: loader.LoadAsync); await _storage.StoreDeployedConfigAsync( instanceName, MakeConfigJson(instanceName), "dep-ok", "rev-ok", true); @@ -510,15 +515,15 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable .ResolveOne(TimeSpan.FromSeconds(5)); /// - /// A deployed-config loader that can be held open mid-read, so a test can commit a - /// lifecycle write while a heal's snapshot is provably in flight rather than hoping the - /// two race the right way round. + /// A single-instance deployed-config loader that can be held open mid-read, so a test can + /// commit a lifecycle write while a heal's snapshot is provably in flight rather than hoping + /// the two race the right way round. /// /// /// Reads real storage every time and only parks AFTER the read, so the returned snapshot is /// exactly what production would have seen at that instant. Inert until - /// is called, which keeps the Deployment Manager's boot load (which runs through this same - /// seam) unblocked. + /// is called, which keeps the Deployment Manager's boot load — a separate seam + /// (_configLoader, register P1-4) untouched by this gate — unblocked. /// /// private sealed class GatedConfigLoader(SiteStorageService storage) @@ -534,16 +539,16 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable public Task WaitUntilParkedAsync() => _parked.Task.WaitAsync(TimeSpan.FromSeconds(10)); - public async Task> LoadAsync() + public async Task LoadAsync(string instanceName) { - var configs = await storage.GetAllDeployedConfigsAsync(); + var config = await storage.GetDeployedConfigAsync(instanceName); var gate = _release; if (gate != null) { _parked.TrySetResult(); await gate.Task; } - return configs; + return config; } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/SiteStorageServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/SiteStorageServiceTests.cs index 77dfcb78..87528b40 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/SiteStorageServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/SiteStorageServiceTests.cs @@ -88,6 +88,80 @@ public class SiteStorageServiceTests : IAsyncLifetime, IDisposable Assert.True(configs[0].IsEnabled); } + // ── Register P1-4: projected single-row reads ── + // + // GetAllDeployedConfigsAsync + FirstOrDefault materializes every deployed config — + // including every ConfigJson blob — to find one row. These lock in that + // GetDeployedConfigAsync (single row, still carries ConfigJson) and + // GetDeployedConfigMetadataAsync (single row, deliberately WITHOUT ConfigJson) return + // exactly the same decision the old full-table-plus-FirstOrDefault path did — the + // defect here is pure I/O cost, not an observable behavior difference, so these tests + // pin the new projection's contract rather than reproduce a behavior bug. + + [Fact] + public async Task GetDeployedConfigAsync_ReturnsSameRowAs_GetAllThenFirstOrDefault() + { + await _storage.StoreDeployedConfigAsync( + "Pump1", "{\"test\":true}", "dep-001", "sha256:abc", isEnabled: true); + await _storage.StoreDeployedConfigAsync( + "Pump2", "{\"other\":1}", "dep-002", "sha256:def", isEnabled: false); + + var viaProjection = await _storage.GetDeployedConfigAsync("Pump1"); + var viaFullScan = (await _storage.GetAllDeployedConfigsAsync()) + .FirstOrDefault(c => c.InstanceUniqueName == "Pump1"); + + Assert.NotNull(viaProjection); + Assert.NotNull(viaFullScan); + Assert.Equal(viaFullScan.InstanceUniqueName, viaProjection.InstanceUniqueName); + Assert.Equal(viaFullScan.ConfigJson, viaProjection.ConfigJson); + Assert.Equal(viaFullScan.DeploymentId, viaProjection.DeploymentId); + Assert.Equal(viaFullScan.RevisionHash, viaProjection.RevisionHash); + Assert.Equal(viaFullScan.IsEnabled, viaProjection.IsEnabled); + Assert.Equal(viaFullScan.DeployedAt, viaProjection.DeployedAt); + } + + [Fact] + public async Task GetDeployedConfigAsync_UnknownInstance_ReturnsNull() + { + await _storage.StoreDeployedConfigAsync( + "Pump1", "{}", "dep-001", "sha256:abc", isEnabled: true); + + var result = await _storage.GetDeployedConfigAsync("NeverHeardOf"); + + Assert.Null(result); + } + + [Fact] + public async Task GetDeployedConfigMetadataAsync_ReturnsSameIdentityAs_GetAllThenFirstOrDefault() + { + await _storage.StoreDeployedConfigAsync( + "Pump1", "{\"test\":true}", "dep-001", "sha256:abc", isEnabled: true); + await _storage.StoreDeployedConfigAsync( + "Pump2", "{\"other\":1}", "dep-002", "sha256:def", isEnabled: false); + + var metadata = await _storage.GetDeployedConfigMetadataAsync("Pump2"); + var viaFullScan = (await _storage.GetAllDeployedConfigsAsync()) + .FirstOrDefault(c => c.InstanceUniqueName == "Pump2"); + + Assert.NotNull(metadata); + Assert.NotNull(viaFullScan); + Assert.Equal(viaFullScan.InstanceUniqueName, metadata.InstanceUniqueName); + Assert.Equal(viaFullScan.DeploymentId, metadata.DeploymentId); + Assert.Equal(viaFullScan.RevisionHash, metadata.RevisionHash); + Assert.Equal(viaFullScan.IsEnabled, metadata.IsEnabled); + } + + [Fact] + public async Task GetDeployedConfigMetadataAsync_UnknownInstance_ReturnsNull() + { + await _storage.StoreDeployedConfigAsync( + "Pump1", "{}", "dep-001", "sha256:abc", isEnabled: true); + + var result = await _storage.GetDeployedConfigMetadataAsync("NeverHeardOf"); + + Assert.Null(result); + } + [Fact] public async Task StoreDeployedConfig_Upserts_OnConflict() {