fix(site-runtime): reconcile artifact deletions on apply — central deletes no longer orphan site rows
The artifact apply (DeploymentManagerActor.HandleDeployArtifacts) was upsert-only, so deleting an external system (or shared script, DB connection, data connection) centrally never removed the site's SQLite row — a deleted external system stayed callable from site scripts forever. Central always ships the COMPLETE set of each artifact class (ArtifactDeploymentService GetAll* snapshots; the wire's presence-tracking wrapper lists preserve null-vs-empty), so the site now applies upsert-then-reconcile: after storing the incoming set, SiteStorageService.DeleteRowsExceptAsync removes any stored row absent from it, per artifact table. A null list still means 'field not shipped' and touches nothing. Runtime cleanup rides along: a reconciled-away shared script is unregistered from the compiled SharedScriptLibrary (a stale delegate would stay callable until restart), and a removed data connection is evicted from the DCL hash cache and its live connection actor stopped via the previously-caller-less RemoveConnectionCommand — both on the actor thread via the extended ApplyArtifactDataConnectionsToDcl message. All four tables are RegisterReplicated, so the deletes reach the standby as ordinary CDC row tombstones. Tests: storage-level reconcile per table (incl. empty-set-deletes-all and idempotency) in ArtifactStorageTests; actor-level pins in DeploymentManagerActorTests (orphan delete, null-set no-op, library unregistration, DCL stop for the removed connection only). Docs: Component-DeploymentManager + Component-SiteRuntime record the full-set/reconcile semantics.
This commit is contained in:
Joseph Doherty
+123
-2
@@ -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<SharedScriptArtifact>? sharedScripts = null,
|
||||
IReadOnlyList<ExternalSystemArtifact>? externalSystems = null,
|
||||
IReadOnlyList<DatabaseConnectionArtifact>? databaseConnections = null,
|
||||
IReadOnlyList<DataConnectionArtifact>? 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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(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<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(10)).Success);
|
||||
dclProbe.ExpectMsg<Commons.Messages.DataConnection.CreateConnectionCommand>(TimeSpan.FromSeconds(5));
|
||||
dclProbe.ExpectMsg<Commons.Messages.DataConnection.CreateConnectionCommand>(TimeSpan.FromSeconds(5));
|
||||
|
||||
manager.Tell(ArtifactsCommand("dep-dc-2", dataConnections:
|
||||
[
|
||||
new DataConnectionArtifact("PlcA", "OpcUa", "{}", null, 3)
|
||||
]));
|
||||
Assert.True(ExpectMsg<ArtifactDeploymentResponse>(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<DataConnectionLayer.Actors.RemoveConnectionCommand>(TimeSpan.FromSeconds(5));
|
||||
Assert.Equal("PlcGone", remove.ConnectionName);
|
||||
Assert.Equal(1, await RowCountAsync("data_connection_definitions"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// In-test fake <see cref="IDeploymentConfigFetcher"/>: returns a canned config JSON
|
||||
/// (notify-and-fetch success) or throws a canned exception (fetch failure), and records
|
||||
|
||||
@@ -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<List<string>> 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<string>();
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user