diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index 1885979f..2f90d2ed 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -177,6 +177,8 @@ A deployment to a site includes the flattened instance configuration plus any sy System-wide artifact deployment is a **separate action** from instance deployment, triggered explicitly by a user with the Deployment role. Artifacts can be deployed to all sites at once or to an individual site (per-site deployment via the Sites admin page). +Each artifact class in the per-site command carries the **complete system-wide set** (the site-scoped full set, for data connections), and the site applies it as **upsert-then-reconcile**: every incoming artifact is stored, then any stored row absent from the set is deleted — the artifact was deleted centrally. This is how central deletes reach sites; there is no per-artifact delete command. Reconciled removals also clean up runtime state on the site: a removed shared script is unregistered from the compiled script library, and a removed data connection's live DCL connection actor is stopped (any deployed instance still referencing it sees bad quality — the standard disconnected signal — until redeployed against current central config). A `null` artifact list on the command means "field not shipped" and touches nothing. + Notification lists and SMTP configuration are **not** deployable artifacts — they are central-only definitions managed by the Notification Service (see Component-NotificationService.md). Notification delivery happens on the central diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 87561c90..1d68ba7c 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -112,6 +112,7 @@ flowchart TD - Receives updated shared scripts, external system definitions, database connection definitions, and data connection definitions from central. (Notification lists and SMTP configuration are central-only and are not deployed to sites — see Component-NotificationService.md.) - Stores all artifacts in local SQLite. After artifact deployment, the site is fully self-contained — all runtime configuration is read from local SQLite with no access to the central configuration database. - Recompiles shared scripts and makes updated code available to all Script Actors. +- **Reconciles deletions**: each non-null artifact list is the complete set for its class, so after upserting, any stored row absent from the set is deleted (the artifact was deleted centrally — there is no per-artifact delete command). A removed shared script is also unregistered from the compiled script library, and a removed data connection's live DCL connection actor is stopped. A `null` list means "field not shipped" and reconciles nothing. ### Instance Lifecycle Commands - **Disable**: Stops the Instance Actor and its children. Retains the deployed configuration in SQLite so the instance can be re-enabled without redeployment. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 56152dc2..2cd221fa 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -1870,6 +1870,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { try { + // Each non-null artifact list is the COMPLETE system-wide set + // (ArtifactDeploymentService ships GetAll* snapshots), so the apply is + // upsert-then-reconcile: store every incoming artifact, then delete any + // stored row absent from the set — that artifact was deleted centrally. + // Upsert-only apply used to leave such rows orphaned on the site forever + // (a centrally-deleted external system stayed callable from site scripts). + // A null list means "field not shipped" and touches nothing. + // Store shared scripts and recompile if (command.SharedScripts != null) { @@ -1881,6 +1889,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Shared scripts recompiled on update _sharedScriptLibrary.CompileAndRegister(script.Name, script.Code); } + + var removedScripts = await _storage.DeleteSharedScriptsExceptAsync( + command.SharedScripts.Select(s => s.Name).ToList()); + foreach (var name in removedScripts) + _sharedScriptLibrary.Remove(name); } // Store external system definitions @@ -1891,6 +1904,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers await _storage.StoreExternalSystemAsync(es.Name, es.EndpointUrl, es.AuthType, es.AuthConfiguration, es.MethodDefinitionsJson, es.TimeoutSeconds); } + + await _storage.DeleteExternalSystemsExceptAsync( + command.ExternalSystems.Select(es => es.Name).ToList()); } // Store database connection definitions @@ -1901,6 +1917,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers await _storage.StoreDatabaseConnectionAsync(db.Name, db.ConnectionString, db.MaxRetries, db.RetryDelay); } + + await _storage.DeleteDatabaseConnectionsExceptAsync( + command.DatabaseConnections.Select(db => db.Name).ToList()); } // Notification lists and SMTP @@ -1923,6 +1942,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers dc.BackupConfigurationJson, dc.FailoverRetryCount); } + var removedConnections = await _storage.DeleteDataConnectionDefinitionsExceptAsync( + command.DataConnections.Select(dc => dc.Name).ToList()); + // After the SQLite store, dispatch an // internal message back to the actor thread so the DCL // push runs through EnsureDclConnection — keeping the @@ -1934,8 +1956,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // self-contained after artifact deployment"). The // helper's hash cache skips unchanged definitions, so // the push is idempotent for re-deploys of the same - // artifact bundle. - self.Tell(new ApplyArtifactDataConnectionsToDcl(command.DataConnections)); + // artifact bundle. Reconciled removals ride the same + // message so the live DCL connection actor is stopped + // on the actor thread alongside the hash-cache eviction. + self.Tell(new ApplyArtifactDataConnectionsToDcl( + command.DataConnections, removedConnections)); } // SMTP configuration is @@ -2012,6 +2037,21 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers dc.BackupConfigurationJson, dc.FailoverRetryCount); } + + // Reconciled removals: the definition row is already gone from SQLite; evict + // the hash cache (so a later re-add with the same name recreates cleanly) and + // stop the live DCL connection actor. A connection still referenced by a + // deployed instance's config no longer exists centrally either — its attributes + // go bad quality, the standard disconnected signal, until the instance is + // redeployed against current central config. + foreach (var name in msg.RemovedConnectionNames) + { + _createdConnections.Remove(name); + _dclManager?.Tell(new DataConnectionLayer.Actors.RemoveConnectionCommand(name)); + _logger.LogWarning( + "Artifact reconcile removed data connection '{Name}' (deleted centrally); " + + "its DCL connection actor has been stopped", name); + } } /// @@ -2118,8 +2158,11 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// 's off-thread persistence task back /// onto the actor thread, so the DCL push (and its hash-cache mutation) /// runs through without crossing - /// thread-confinement boundaries. + /// thread-confinement boundaries. + /// carries the reconcile-deleted definitions so the hash-cache eviction and + /// live DCL connection stop also happen actor-thread-confined. /// internal record ApplyArtifactDataConnectionsToDcl( - IReadOnlyList DataConnections); + IReadOnlyList DataConnections, + IReadOnlyList RemovedConnectionNames); } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs index 8dd80e06..1f97e8c4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs @@ -760,6 +760,93 @@ public class SiteStorageService return results; } + + // ── Artifact set reconciliation ── + + /// + /// Deletes shared scripts absent from the supplied full set and returns the deleted names. + /// + /// The complete set of shared script names that should exist. + /// A task that resolves to the names of the rows that were deleted. + public Task> DeleteSharedScriptsExceptAsync(IReadOnlyCollection keepNames) + => DeleteRowsExceptAsync("shared_scripts", keepNames); + + /// + /// Deletes external system definitions absent from the supplied full set and returns the deleted names. + /// + /// The complete set of external system names that should exist. + /// A task that resolves to the names of the rows that were deleted. + public Task> DeleteExternalSystemsExceptAsync(IReadOnlyCollection keepNames) + => DeleteRowsExceptAsync("external_systems", keepNames); + + /// + /// Deletes database connection definitions absent from the supplied full set and returns the deleted names. + /// + /// The complete set of database connection names that should exist. + /// A task that resolves to the names of the rows that were deleted. + public Task> DeleteDatabaseConnectionsExceptAsync(IReadOnlyCollection keepNames) + => DeleteRowsExceptAsync("database_connections", keepNames); + + /// + /// Deletes data connection definitions absent from the supplied full set and returns the deleted names. + /// + /// The complete set of data connection names that should exist. + /// A task that resolves to the names of the rows that were deleted. + public Task> DeleteDataConnectionDefinitionsExceptAsync(IReadOnlyCollection keepNames) + => DeleteRowsExceptAsync("data_connection_definitions", keepNames); + + /// + /// Deletes every row of an artifact table whose name is not in + /// , returning the deleted names. + /// + /// Central always ships the COMPLETE system-wide set of each artifact class on an + /// artifact deployment, so a stored row absent from the incoming set means the + /// artifact was deleted centrally. The store methods are upsert-only, which used to + /// leave such rows orphaned on the site forever (a centrally-deleted external + /// system stayed callable from site scripts indefinitely); the artifact apply now + /// reconciles by calling this after upserting. An empty + /// legitimately deletes every row — central saying "none of these exist anymore". + /// + private async Task> DeleteRowsExceptAsync(string table, IReadOnlyCollection keepNames) + { + await using var connection = OpenConnection(); + await using var transaction = connection.BeginTransaction(); + + var keepParams = keepNames.Select((name, i) => (Key: $"@k{i}", Value: name)).ToList(); + var predicate = keepParams.Count == 0 + ? string.Empty + : $" WHERE name NOT IN ({string.Join(", ", keepParams.Select(p => p.Key))})"; + + var removed = new List(); + await using (var select = connection.CreateCommand()) + { + select.Transaction = transaction; + select.CommandText = $"SELECT name FROM {table}{predicate}"; + foreach (var (key, value) in keepParams) + select.Parameters.AddWithValue(key, value); + + await using var reader = await select.ExecuteReaderAsync(); + while (await reader.ReadAsync()) + removed.Add(reader.GetString(0)); + } + + if (removed.Count > 0) + { + await using var delete = connection.CreateCommand(); + delete.Transaction = transaction; + delete.CommandText = $"DELETE FROM {table}{predicate}"; + foreach (var (key, value) in keepParams) + delete.Parameters.AddWithValue(key, value); + + await delete.ExecuteNonQueryAsync(); + _logger.LogInformation( + "Artifact reconcile removed {Count} orphaned row(s) from {Table}: {Names}", + removed.Count, table, string.Join(", ", removed)); + } + + transaction.Commit(); + return removed; + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorTests.cs index 4f448aa0..9d1258ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorTests.cs @@ -57,7 +57,7 @@ public class DeploymentManagerActorTests : TestKit, IDisposable private IActorRef CreateDeploymentManager( SiteRuntimeOptions? options = null, IServiceProvider? serviceProvider = null, - IDeploymentConfigFetcher? configFetcher = null) + IDeploymentConfigFetcher? configFetcher = null, IActorRef? dclManager = null) { options ??= new SiteRuntimeOptions(); return ActorOf(Props.Create(() => new DeploymentManagerActor( @@ -70,7 +70,7 @@ public class DeploymentManagerActorTests : TestKit, IDisposable // Named from here on. These trailing parameters are all optional and several // share a type, so a positional list silently binds the wrong argument when the // signature changes — which is exactly what removing replicationActor did. - dclManager: null, + dclManager: dclManager, healthCollector: null, serviceProvider: serviceProvider, loggerFactory: null, @@ -971,6 +971,127 @@ public class DeploymentManagerActorTests : TestKit, IDisposable "The plaintext SMTP password is still on disk."); } + // ── Artifact set reconciliation (orphan cleanup) ── + // + // Central ships the COMPLETE system-wide set of each artifact class on every + // artifact deployment, so the apply must delete stored rows absent from the + // incoming set — they were deleted centrally. The apply used to be upsert-only, + // leaving e.g. a centrally-deleted external system callable from site scripts + // forever. ArtifactStorageTests covers the storage deletes in isolation; these + // tests pin the ACTOR's calls to them (and the runtime cleanup that must ride + // along: shared-script library unregistration, DCL connection stop). + + private static DeployArtifactsCommand ArtifactsCommand( + string deploymentId, + IReadOnlyList? sharedScripts = null, + IReadOnlyList? externalSystems = null, + IReadOnlyList? databaseConnections = null, + IReadOnlyList? dataConnections = null) + => new( + DeploymentId: deploymentId, + SharedScripts: sharedScripts, + ExternalSystems: externalSystems, + DatabaseConnections: databaseConnections, + NotificationLists: null, + DataConnections: dataConnections, + SmtpConfigurations: null, + Timestamp: DateTimeOffset.UtcNow); + + [Fact] + public async Task ApplyingArtifacts_DeletesExternalSystemsAbsentFromTheSet() + { + var manager = CreateDeploymentManager(); + + manager.Tell(ArtifactsCommand("dep-es-1", externalSystems: + [ + new ExternalSystemArtifact("MES", "https://mes", "ApiKey", null, null, 0), + new ExternalSystemArtifact("Legacy", "https://legacy", "Basic", null, null, 0) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + Assert.Equal(2, await RowCountAsync("external_systems")); + + // Second deployment: "Legacy" was deleted centrally, so the full set is just "MES". + manager.Tell(ArtifactsCommand("dep-es-2", externalSystems: + [ + new ExternalSystemArtifact("MES", "https://mes", "ApiKey", null, null, 0) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + + Assert.Equal(1, await RowCountAsync("external_systems")); + } + + [Fact] + public async Task ApplyingArtifacts_NullSet_TouchesNothing() + { + var manager = CreateDeploymentManager(); + + manager.Tell(ArtifactsCommand("dep-null-1", externalSystems: + [ + new ExternalSystemArtifact("MES", "https://mes", "ApiKey", null, null, 0) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + + // A null list means "field not shipped", NOT "empty set" — it must not reconcile. + manager.Tell(ArtifactsCommand("dep-null-2", externalSystems: null)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + + Assert.Equal(1, await RowCountAsync("external_systems")); + } + + [Fact] + public async Task ApplyingArtifacts_RemovedSharedScript_IsUnregisteredFromLibrary() + { + var manager = CreateDeploymentManager(); + + manager.Tell(ArtifactsCommand("dep-ss-1", sharedScripts: + [ + new SharedScriptArtifact("KeepScript", "return 1;", null, null), + new SharedScriptArtifact("GoneScript", "return 2;", null, null) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + Assert.True(_sharedScriptLibrary.Contains("GoneScript")); + + manager.Tell(ArtifactsCommand("dep-ss-2", sharedScripts: + [ + new SharedScriptArtifact("KeepScript", "return 1;", null, null) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + + // Both the SQLite row and the compiled registration must be gone — a stale + // compiled delegate would keep the deleted script callable until restart. + Assert.Equal(1, await RowCountAsync("shared_scripts")); + Assert.True(_sharedScriptLibrary.Contains("KeepScript")); + Assert.False(_sharedScriptLibrary.Contains("GoneScript")); + } + + [Fact] + public async Task ApplyingArtifacts_RemovedDataConnection_StopsLiveDclConnection() + { + var dclProbe = CreateTestProbe(); + var manager = CreateDeploymentManager(dclManager: dclProbe.Ref); + + manager.Tell(ArtifactsCommand("dep-dc-1", dataConnections: + [ + new DataConnectionArtifact("PlcA", "OpcUa", "{}", null, 3), + new DataConnectionArtifact("PlcGone", "OpcUa", "{}", null, 3) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + dclProbe.ExpectMsg(TimeSpan.FromSeconds(5)); + dclProbe.ExpectMsg(TimeSpan.FromSeconds(5)); + + manager.Tell(ArtifactsCommand("dep-dc-2", dataConnections: + [ + new DataConnectionArtifact("PlcA", "OpcUa", "{}", null, 3) + ])); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(10)).Success); + + // "PlcA" is unchanged (hash-cache skip), so the next DCL message must be the + // stop for the reconciled-away "PlcGone" — and the definition row is gone too. + var remove = dclProbe.ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("PlcGone", remove.ConnectionName); + Assert.Equal(1, await RowCountAsync("data_connection_definitions")); + } + /// /// In-test fake : returns a canned config JSON /// (notify-and-fetch success) or throws a canned exception (fetch failure), and records diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/ArtifactStorageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/ArtifactStorageTests.cs index 997c244b..a8cb4bb4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/ArtifactStorageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Persistence/ArtifactStorageTests.cs @@ -131,6 +131,88 @@ public class ArtifactStorageTests : IAsyncLifetime, IDisposable // Upsert should not throw } + // ── Artifact set reconciliation ── + // + // Central always ships the COMPLETE system-wide set of each artifact class, so a + // stored row absent from the incoming set was deleted centrally. The store methods + // are upsert-only; without these deletes a centrally-deleted external system (or + // shared script / DB connection / data connection) stayed orphaned on the site + // forever and remained callable from site scripts. + + [Fact] + public async Task DeleteSharedScriptsExcept_RemovesOrphans_KeepsPresent() + { + await _storage.StoreSharedScriptAsync("Keep1", "1", null, null); + await _storage.StoreSharedScriptAsync("Keep2", "2", null, null); + await _storage.StoreSharedScriptAsync("Orphan", "3", null, null); + + var removed = await _storage.DeleteSharedScriptsExceptAsync(["Keep1", "Keep2"]); + + Assert.Equal(["Orphan"], removed); + var names = (await _storage.GetAllSharedScriptsAsync()).Select(s => s.Name).Order().ToList(); + Assert.Equal(["Keep1", "Keep2"], names); + } + + [Fact] + public async Task DeleteExternalSystemsExcept_RemovesOrphans_KeepsPresent() + { + await _storage.StoreExternalSystemAsync("MES", "https://mes", "ApiKey", null, null); + await _storage.StoreExternalSystemAsync("Deleted", "https://old", "Basic", null, null); + + var removed = await _storage.DeleteExternalSystemsExceptAsync(["MES"]); + + Assert.Equal(["Deleted"], removed); + Assert.Equal(["MES"], await TableNamesAsync("external_systems")); + } + + [Fact] + public async Task DeleteDatabaseConnectionsExcept_EmptyKeepSet_DeletesAll() + { + await _storage.StoreDatabaseConnectionAsync("DB1", "Server=a", 3, TimeSpan.FromSeconds(1)); + await _storage.StoreDatabaseConnectionAsync("DB2", "Server=b", 3, TimeSpan.FromSeconds(1)); + + // An empty full set is legitimate: central saying no DB connections exist anymore. + var removed = await _storage.DeleteDatabaseConnectionsExceptAsync([]); + + Assert.Equal(["DB1", "DB2"], removed.Order().ToList()); + Assert.Empty(await TableNamesAsync("database_connections")); + } + + [Fact] + public async Task DeleteDataConnectionDefinitionsExcept_RemovesOrphans_KeepsPresent() + { + await _storage.StoreDataConnectionDefinitionAsync("PlcA", "OpcUa", "{}"); + await _storage.StoreDataConnectionDefinitionAsync("Gone", "OpcUa", "{}"); + + var removed = await _storage.DeleteDataConnectionDefinitionsExceptAsync(["PlcA"]); + + Assert.Equal(["Gone"], removed); + var names = (await _storage.GetAllDataConnectionDefinitionsAsync()).Select(d => d.Name).ToList(); + Assert.Equal(["PlcA"], names); + } + + [Fact] + public async Task DeleteRowsExcept_NoOrphans_ReturnsEmpty_AndIsIdempotent() + { + await _storage.StoreExternalSystemAsync("MES", "https://mes", "ApiKey", null, null); + + Assert.Empty(await _storage.DeleteExternalSystemsExceptAsync(["MES"])); + Assert.Empty(await _storage.DeleteExternalSystemsExceptAsync(["MES"])); + Assert.Equal(["MES"], await TableNamesAsync("external_systems")); + } + + private async Task> TableNamesAsync(string table) + { + await using var connection = _storage.CreateConnection(); + await using var command = connection.CreateCommand(); + command.CommandText = $"SELECT name FROM {table} ORDER BY name"; + var names = new List(); + await using var reader = await command.ExecuteReaderAsync(); + while (await reader.ReadAsync()) + names.Add(reader.GetString(0)); + return names; + } + // ── DeploymentManager-025 / SiteRuntime-031: central-only notif/SMTP purge ── // // Notification config is central-only. The site-side write paths and