From 099b2221d62b7abf0769ea060a9598baa8da3632 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 09:05:07 -0400 Subject: [PATCH] fix(dcl): ASCII-only connection actor names, guarded ActorOf, and gate rejection of colliding connection names (review 2026-10-21 lane A M2) ConnectionActorNames.Sanitize (shared by the DCL manager and the site deploy gate) keeps ASCII letters/digits and -_.*+:@&=,!~';() only. The manager fails a connection whose sanitised name is invalid or already used (Error + site event) instead of restarting; DeployCompileValidator rejects a config whose connection names collide after sanitising. --- docs/requirements/Component-SiteRuntime.md | 2 +- .../Actors/ConnectionActorNames.cs | 43 +++++++++ .../Actors/DataConnectionManagerActor.cs | 36 +++++++- .../Deployment/DeployCompileValidator.cs | 22 +++++ .../Actors/ConnectionActorNameTests.cs | 92 +++++++++++++++++++ .../Deployment/DeployCompileValidatorTests.cs | 21 +++++ 6 files changed, 210 insertions(+), 6 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/ConnectionActorNames.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/ConnectionActorNameTests.cs diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index d0abf84a..364c8cf5 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -162,7 +162,7 @@ flowchart TD > **A deploy's config is deserialized once (review 2026-10-21 01/SR-P5).** The compile warm, the synchronous gate, the DCL-connection ensure and the Instance Actor constructor each used to `JsonSerializer.Deserialize` the same ~70 KB JSON — four object graphs per instance, two of them on the singleton's own thread. The warm now parses it once off-thread (`DeployCompileValidator.TryParse`), and the parsed object rides `DeployCompileWarmed` → the gate (`Validate(FlattenedConfiguration, …)`) → the redeploy buffer → `CreateInstanceActor` → a new `InstanceActor` constructor overload that takes the parsed configuration (read-only, so sharing it is safe; the Props no longer retain the JSON string either). A config that does not parse is still the gate's readable `Configuration JSON failed to deserialize` rejection. Paths that start from a stored row (enable, self-heal, boot) parse once inside `CreateInstanceActor`; a stored row that does not parse is handed to the actor as JSON, so it still fails init through the S6 path exactly as before. -> **Runtime names are validated before anything is applied (review 2026-10-21 01/SR-C1).** The instance unique name becomes the Instance Actor's name, and every script / alarm / native-alarm-source canonical name becomes a child actor name (`script-…`, `alarm-…`, `native-alarm-…`). Akka refuses any of these that is not a legal path element (a space, `#`, `/`, `[`, `]`, a bare `%`, a non-ASCII letter, a leading `$`), and the refusal is a **synchronous** `InvalidActorNameException` from `Context.ActorOf` — on the Deployment Manager singleton's own thread for an instance name, where the default Restart directive used to stop **every** Instance Actor on the site, lose all in-flight deploy bookkeeping, and leave the deployer's Ask to time out (so a retry repeated the outage). The site compile gate (`DeployCompileValidator`) now checks all of those names with Akka's own `ActorPath.IsValidPathElement` and rejects the deploy with `Site deploy validation failed: Invalid runtime name: …` before any reset, persistence or actor work. It deliberately does **not** decide with the Commons mirror `ActorNameRules`: the mirror is currently stricter than Akka (it refuses `"`, `(` and `)`), so gating on it would reject names that deploy today. Behind the gate, `CreateInstanceActor` catches the exception and returns a failure that each caller reports — a Failed deploy, a failed enable, an Error site event for a self-heal start, a skipped instance in the boot batch — so a row stored before names were checked can never escalate to the singleton. The Instance Actor applies the same rule to its children: a refused child name is logged and that one child skipped, exactly like a compile failure, instead of failing the whole instance's `PreStart`. +> **Runtime names are validated before anything is applied (review 2026-10-21 01/SR-C1).** The instance unique name becomes the Instance Actor's name, and every script / alarm / native-alarm-source canonical name becomes a child actor name (`script-…`, `alarm-…`, `native-alarm-…`). Akka refuses any of these that is not a legal path element (a space, `#`, `/`, `[`, `]`, a bare `%`, a non-ASCII letter, a leading `$`), and the refusal is a **synchronous** `InvalidActorNameException` from `Context.ActorOf` — on the Deployment Manager singleton's own thread for an instance name, where the default Restart directive used to stop **every** Instance Actor on the site, lose all in-flight deploy bookkeeping, and leave the deployer's Ask to time out (so a retry repeated the outage). The site compile gate (`DeployCompileValidator`) now checks all of those names with Akka's own `ActorPath.IsValidPathElement` and rejects the deploy with `Site deploy validation failed: Invalid runtime name: …` before any reset, persistence or actor work. It deliberately does **not** decide with the Commons mirror `ActorNameRules`: the mirror is currently stricter than Akka (it refuses `"`, `(` and `)`), so gating on it would reject names that deploy today. Behind the gate, `CreateInstanceActor` catches the exception and returns a failure that each caller reports — a Failed deploy, a failed enable, an Error site event for a self-heal start, a skipped instance in the boot batch — so a row stored before names were checked can never escalate to the singleton. **Data connection names** become `DataConnectionActor` names through the DCL's shared sanitiser `ConnectionActorNames.Sanitize` (ASCII letters/digits plus `-_.*+:@&=,!~';()`, everything else → `-`, so the result is always Akka-legal). Because that mapping is not injective (`PLC A` and `PLC-A` both become `PLC-A`), the gate also rejects a config whose connection names collide after sanitising, and the DCL manager fails just the later connection (Error log + `connection` site event) instead of letting `ActorOf` restart it — the guard that also covers collisions between connections from different instances, which a per-instance gate cannot see. The Instance Actor applies the same rule to its children: a refused child name is logged and that one child skipped, exactly like a compile failure, instead of failing the whole instance's `PreStart`. ### System-Wide Artifact Handling - 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.) diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/ConnectionActorNames.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/ConnectionActorNames.cs new file mode 100644 index 00000000..e4bb9be9 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/ConnectionActorNames.cs @@ -0,0 +1,43 @@ +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors; + +/// +/// Maps a data connection name to the Akka actor name of its +/// (review 2026-10-21, lane A M2). Shared by (which +/// creates the actor) and the site deploy gate (which rejects a config whose connection names +/// would collide once sanitised), so both always agree on the name. +/// +/// +/// Every output is a legal Akka path element: ASCII letters and digits plus +/// -_.*+:@&=,!~';() are kept, everything else — spaces, /, #, $ +/// and every non-ASCII character — becomes -. The previous sanitiser kept +/// (true for non-ASCII letters, which Akka refuses) and +/// $ (refused as a leading character), so "Ofen Süd" or "$PLC" threw out of +/// Context.ActorOf. The mapping is NOT injective ("PLC A" and "PLC-A" both give +/// "PLC-A"); callers must treat a collision as an error for the later connection. +/// +/// +public static class ConnectionActorNames +{ + private const string KeptSymbols = "-_.*+:@&=,!~';()"; + + /// The actor name used for . + /// The data connection's configured name. + /// A legal, non-empty Akka actor name. + public static string Sanitize(string connectionName) + { + if (string.IsNullOrEmpty(connectionName)) + return "-"; + + var chars = new char[connectionName.Length]; + for (var i = 0; i < chars.Length; i++) + { + var c = connectionName[i]; + chars[i] = (c is >= 'a' and <= 'z' or >= 'A' and <= 'Z' or >= '0' and <= '9') || + KeptSymbols.Contains(c) + ? c + : '-'; + } + + return new string(chars); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs index f7479543..0bc58411 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionManagerActor.cs @@ -92,11 +92,37 @@ public class DataConnectionManagerActor : ReceiveActor command.FailoverRetryCount, _siteEventLogger)); - // Sanitize name for Akka actor path (replace spaces and invalid chars) - var actorName = new string(command.ConnectionName - .Select(c => char.IsLetterOrDigit(c) || "-_.*$+:@&=,!~';()".Contains(c) ? c : '-') - .ToArray()); - var actorRef = Context.ActorOf(props, actorName); + // Sanitize name for Akka actor path — ASCII only, never a leading '$' (see + // ConnectionActorNames). Two connection names can still sanitise to the same actor name + // ("PLC A" / "PLC-A"), and ActorOf throws synchronously on that (or on any name Akka + // refuses): that must fail THIS connection only, never escape to the supervisor and + // restart the whole manager with every live connection (review 2026-10-21, lane A M2). + var actorName = ConnectionActorNames.Sanitize(command.ConnectionName); + IActorRef actorRef; + try + { + if (!Context.Child(actorName).IsNobody()) + throw new InvalidActorNameException( + $"actor name [{actorName}] is already used by another connection"); + actorRef = Context.ActorOf(props, actorName); + } + catch (InvalidActorNameException ex) + { + _log.Error( + "Connection {0} not created: its actor name '{1}' is invalid or already used by another " + + "connection ({2}). Rename the connection so its name differs in more than spaces/symbols.", + command.ConnectionName, actorName, ex.Message); + if (_siteEventLogger != null) + { + _ = _siteEventLogger.LogEventAsync( + "connection", "Error", null, command.ConnectionName, + $"Connection not created — runtime actor name '{actorName}' is invalid or already in use", + ex.Message); + } + adapter.DisposeAsync().AsTask().ContinueWith(_ => { }); + return; + } + _connectionActors[command.ConnectionName] = actorRef; _log.Info("Created DataConnectionActor for {0} (protocol={1})", diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs index 1b005753..a59360c0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs @@ -246,6 +246,28 @@ public sealed class DeployCompileValidator foreach (var source in config.NativeAlarmSources) AddActorNameError(errors, "Native alarm source", source.CanonicalName, $"native-alarm-{source.CanonicalName}"); + + // Data connection names become DataConnectionActor names through the DCL's sanitiser, + // which is not injective: "PLC A" and "PLC-A" both map to "PLC-A", and the second + // connection could never be created (lane A M2). Reject such a pair here, by name. + if (config.Connections is { Count: > 0 } connections) + { + var byActorName = new Dictionary(StringComparer.Ordinal); + foreach (var name in connections.Keys.Order(StringComparer.Ordinal)) + { + var actorName = DataConnectionLayer.Actors.ConnectionActorNames.Sanitize(name); + if (byActorName.TryGetValue(actorName, out var other)) + { + errors.Add($"{ActorNameErrorPrefix}Data connection names '{other}' and '{name}' " + + $"both map to the runtime actor name '{actorName}' — rename one so they " + + "differ in more than spaces or symbols."); + continue; + } + + byActorName[actorName] = name; + AddActorNameError(errors, "Data connection", name, actorName); + } + } } private static void AddActorNameError( diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/ConnectionActorNameTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/ConnectionActorNameTests.cs new file mode 100644 index 00000000..2d7dff99 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/ConnectionActorNameTests.cs @@ -0,0 +1,92 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.DataConnection; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors; +using ZB.MOM.WW.ScadaBridge.HealthMonitoring; + +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests.Actors; + +/// +/// Review 2026-10-21 (lane A M2): a data connection name becomes its DataConnectionActor's +/// name. The old sanitiser kept non-ASCII letters and '$' (both refused by Akka) and mapped +/// "PLC A" and "PLC-A" to the same name; either made Context.ActorOf throw on the +/// manager's own thread, restarting it and dropping every live connection. +/// +[Collection("DataConnectionManagerActor")] +public class ConnectionActorNameTests : TestKit +{ + private readonly IDataConnectionFactory _factory = Substitute.For(); + private readonly ISiteHealthCollector _health = Substitute.For(); + private readonly DataConnectionOptions _options = new() + { + ReconnectInterval = TimeSpan.FromMilliseconds(100), + TagResolutionRetryInterval = TimeSpan.FromMilliseconds(200) + }; + + /// Creates the test kit with a factory that hands out connected mock adapters. + public ConnectionActorNameTests() + { + _factory.Create(Arg.Any(), Arg.Any>()).Returns(_ => + { + var adapter = Substitute.For(); + adapter.ConnectAsync(Arg.Any>(), Arg.Any()) + .Returns(Task.CompletedTask); + adapter.Status.Returns(ConnectionHealth.Connected); + adapter.SubscribeAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns("sub-1"); + return adapter; + }); + } + + /// Every sanitised name is a legal Akka path element, including non-ASCII and '$' input. + [Theory] + [InlineData("PLC A")] + [InlineData("Ofen Süd")] + [InlineData("$PLC")] + [InlineData("名前")] + [InlineData("a/b#c[d]%e")] + [InlineData("")] + public void Sanitize_AlwaysYieldsAValidActorName(string connectionName) + { + Assert.True(ActorPath.IsValidPathElement(ConnectionActorNames.Sanitize(connectionName))); + } + + private IActorRef CreateManager() => Sys.ActorOf(Props.Create(() => + new DataConnectionManagerActor(_factory, _options, _health))); + + private SubscribeTagsResponse Subscribe(IActorRef manager, string connection) + { + manager.Tell(new SubscribeTagsRequest( + Guid.NewGuid().ToString("N"), "inst1", connection, ["tag1"], DateTimeOffset.UtcNow)); + return ExpectMsg(TimeSpan.FromSeconds(10)); + } + + /// A non-ASCII connection name is created instead of crashing the manager. + [Fact] + public void NonAsciiConnectionName_IsCreated() + { + var manager = CreateManager(); + manager.Tell(new CreateConnectionCommand("Ofen Süd", "OpcUa", new Dictionary(), null, 3)); + + var response = Subscribe(manager, "Ofen Süd"); + Assert.DoesNotContain("Unknown connection", response.ErrorMessage ?? ""); + } + + /// + /// Two names that sanitise alike: the second fails alone; the first — and the manager — + /// keep working. + /// + [Fact] + public void CollidingSanitisedNames_FailOnlyTheLaterConnection() + { + var manager = CreateManager(); + manager.Tell(new CreateConnectionCommand("PLC A", "OpcUa", new Dictionary(), null, 3)); + manager.Tell(new CreateConnectionCommand("PLC-A", "OpcUa", new Dictionary(), null, 3)); + + Assert.Contains("Unknown connection", Subscribe(manager, "PLC-A").ErrorMessage); + Assert.DoesNotContain("Unknown connection", Subscribe(manager, "PLC A").ErrorMessage ?? ""); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs index 09c2ecff..d2fc2e96 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs @@ -143,4 +143,25 @@ public class DeployCompileValidatorTests Assert.Contains(nameErrors, e => e.Contains("Hi/Temp")); Assert.Contains(nameErrors, e => e.Contains("Src#1")); } + + /// + /// Lane A M2: connection names that sanitise to the same DataConnectionActor name are + /// rejected by the gate; distinct ones (including non-ASCII) pass. + /// + [Fact] + public void ConnectionNames_CollidingAfterSanitising_AreRejected() + { + static string Cfg(params string[] names) => JsonSerializer.Serialize(new FlattenedConfiguration + { + Scripts = [], Alarms = [], NativeAlarmSources = [], Attributes = [], + Connections = names.ToDictionary(n => n, _ => new ConnectionConfig { Protocol = "OpcUa" }) + }); + + var collide = NewValidator().Validate(Cfg("PLC A", "PLC-A")).Where(DeployCompileValidator.IsActorNameError).ToList(); + Assert.Single(collide); + Assert.Contains("PLC A", collide[0]); + Assert.Contains("PLC-A", collide[0]); + + Assert.Empty(NewValidator().Validate(Cfg("PLC A", "Ofen Süd"))); + } }