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.
2026-10-22 09:05:07 -04:00
parent 3c11510c52
commit 099b2221d6
6 changed files with 210 additions and 6 deletions
@@ -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<FlattenedConfiguration>` 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.)
@@ -0,0 +1,43 @@
+namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Actors;
+
+/// <summary>
+/// Maps a data connection name to the Akka actor name of its <see cref="DataConnectionActor"/>
+/// (review 2026-10-21, lane A M2). Shared by <see cref="DataConnectionManagerActor"/> (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.
+///
+/// <para>
+/// Every output is a legal Akka path element: ASCII letters and digits plus
+/// <c>-_.*+:@&amp;=,!~';()</c> are kept, everything else — spaces, <c>/</c>, <c>#</c>, <c>$</c>
+/// and every non-ASCII character — becomes <c>-</c>. The previous sanitiser kept
+/// <see cref="char.IsLetterOrDigit(char)"/> (true for non-ASCII letters, which Akka refuses) and
+/// <c>$</c> (refused as a leading character), so "Ofen Süd" or "$PLC" threw out of
+/// <c>Context.ActorOf</c>. 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.
+/// </para>
+/// </summary>
+public static class ConnectionActorNames
+{
+    private const string KeptSymbols = "-_.*+:@&=,!~';()";
+
+    /// <summary>The actor name used for <paramref name="connectionName"/>.</summary>
+    /// <param name="connectionName">The data connection's configured name.</param>
+    /// <returns>A legal, non-empty Akka actor name.</returns>
+    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);
+    }
+}
@@ -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})",
@@ -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<string, string>(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(
@@ -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;
+
+/// <summary>
+/// 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 <c>Context.ActorOf</c> throw on the
+/// manager's own thread, restarting it and dropping every live connection.
+/// </summary>
+[Collection("DataConnectionManagerActor")]
+public class ConnectionActorNameTests : TestKit
+{
+    private readonly IDataConnectionFactory _factory = Substitute.For<IDataConnectionFactory>();
+    private readonly ISiteHealthCollector _health = Substitute.For<ISiteHealthCollector>();
+    private readonly DataConnectionOptions _options = new()
+    {
+        ReconnectInterval = TimeSpan.FromMilliseconds(100),
+        TagResolutionRetryInterval = TimeSpan.FromMilliseconds(200)
+    };
+
+    /// <summary>Creates the test kit with a factory that hands out connected mock adapters.</summary>
+    public ConnectionActorNameTests()
+    {
+        _factory.Create(Arg.Any<string>(), Arg.Any<IDictionary<string, string>>()).Returns(_ =>
+        {
+            var adapter = Substitute.For<IDataConnection>();
+            adapter.ConnectAsync(Arg.Any<IDictionary<string, string>>(), Arg.Any<CancellationToken>())
+                .Returns(Task.CompletedTask);
+            adapter.Status.Returns(ConnectionHealth.Connected);
+            adapter.SubscribeAsync(Arg.Any<string>(), Arg.Any<SubscriptionCallback>(), Arg.Any<CancellationToken>())
+                .Returns("sub-1");
+            return adapter;
+        });
+    }
+
+    /// <summary>Every sanitised name is a legal Akka path element, including non-ASCII and '$' input.</summary>
+    [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<SubscribeTagsResponse>(TimeSpan.FromSeconds(10));
+    }
+
+    /// <summary>A non-ASCII connection name is created instead of crashing the manager.</summary>
+    [Fact]
+    public void NonAsciiConnectionName_IsCreated()
+    {
+        var manager = CreateManager();
+        manager.Tell(new CreateConnectionCommand("Ofen Süd", "OpcUa", new Dictionary<string, string>(), null, 3));
+
+        var response = Subscribe(manager, "Ofen Süd");
+        Assert.DoesNotContain("Unknown connection", response.ErrorMessage ?? "");
+    }
+
+    /// <summary>
+    /// Two names that sanitise alike: the second fails alone; the first — and the manager —
+    /// keep working.
+    /// </summary>
+    [Fact]
+    public void CollidingSanitisedNames_FailOnlyTheLaterConnection()
+    {
+        var manager = CreateManager();
+        manager.Tell(new CreateConnectionCommand("PLC A", "OpcUa", new Dictionary<string, string>(), null, 3));
+        manager.Tell(new CreateConnectionCommand("PLC-A", "OpcUa", new Dictionary<string, string>(), null, 3));
+
+        Assert.Contains("Unknown connection", Subscribe(manager, "PLC-A").ErrorMessage);
+        Assert.DoesNotContain("Unknown connection", Subscribe(manager, "PLC A").ErrorMessage ?? "");
+    }
+}
@@ -143,4 +143,25 @@ public class DeployCompileValidatorTests
        Assert.Contains(nameErrors, e => e.Contains("Hi/Temp"));
        Assert.Contains(nameErrors, e => e.Contains("Src#1"));
    }
+
+    /// <summary>
+    /// Lane A M2: connection names that sanitise to the same DataConnectionActor name are
+    /// rejected by the gate; distinct ones (including non-ASCII) pass.
+    /// </summary>
+    [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")));
+    }
 }