fix(communication): generation-suffixed, sanitized ClusterClient actor names to prevent recreate name collision

2026-07-08 14:55:05 -04:00
parent 7231c2044b
commit 75cbac4478
2 changed files with 79 additions and 1 deletions
@@ -32,11 +32,35 @@ public interface ISiteClientFactory
 /// </summary>
 public class DefaultSiteClientFactory : ISiteClientFactory
 {
+    /// <summary>
+    /// Per-incarnation generation counter. Context.Stop of the previous client is
+    /// asynchronous — its actor name stays reserved until termination completes —
+    /// so a same-named recreate in the same message handling throws
+    /// InvalidActorNameException. A generation suffix makes every incarnation's
+    /// name unique by construction (mirrors SiteStreamGrpcServer._actorCounter).
+    /// </summary>
+    private long _generation;
+
    /// <inheritdoc />
    public IActorRef Create(ActorSystem system, string siteId, ImmutableHashSet<ActorPath> contacts)
    {
        var settings = ClusterClientSettings.Create(system).WithInitialContacts(contacts);
-        return system.ActorOf(ClusterClient.Props(settings), $"site-client-{siteId}");
+        var name = $"site-client-{SanitizeForActorName(siteId)}-{System.Threading.Interlocked.Increment(ref _generation)}";
+        return system.ActorOf(ClusterClient.Props(settings), name);
+    }
+
+    /// <summary>
+    /// Maps an arbitrary SiteIdentifier onto a valid Akka actor-path element:
+    /// letters/digits/'-'/'_' pass through, everything else becomes '_'. Uniqueness
+    /// is NOT required here (the generation suffix guarantees it); only validity is.
+    /// </summary>
+    internal static string SanitizeForActorName(string siteId)
+    {
+        if (string.IsNullOrEmpty(siteId)) return "site";
+        var sanitized = new string(siteId
+            .Select(c => char.IsAsciiLetterOrDigit(c) || c is '-' or '_' ? c : '_')
+            .ToArray());
+        return ActorPath.IsValidPathElement(sanitized) ? sanitized : "site";
    }
 }

@@ -0,0 +1,54 @@
+using System.Collections.Immutable;
+using Akka.Actor;
+using Akka.Cluster.Tools.Client;
+using Akka.Configuration;
+using Akka.TestKit.Xunit2;
+using Xunit;
+using ZB.MOM.WW.ScadaBridge.Communication.Actors;
+
+namespace ZB.MOM.WW.ScadaBridge.Communication.Tests;
+
+public class DefaultSiteClientFactoryTests : TestKit
+{
+    private static readonly Config TestConfig = ConfigurationFactory.ParseString(@"
+        akka.actor.provider = cluster
+        akka.remote.dot-netty.tcp.port = 0
+        akka.remote.dot-netty.tcp.hostname = localhost")
+        .WithFallback(ClusterClientReceptionist.DefaultConfig());
+
+    public DefaultSiteClientFactoryTests() : base(TestConfig) { }
+
+    private static ImmutableHashSet<ActorPath> Contacts() =>
+        ImmutableHashSet.Create(ActorPath.Parse("akka.tcp://other@localhost:2552/system/receptionist"));
+
+    [Fact]
+    public void Create_TwiceForSameSite_DoesNotCollide()
+    {
+        var factory = new DefaultSiteClientFactory();
+        var first = factory.Create(Sys, "site-a", Contacts());
+        var second = factory.Create(Sys, "site-a", Contacts()); // pre-fix: InvalidActorNameException
+        Assert.NotEqual(first.Path, second.Path);
+    }
+
+    [Theory]
+    [InlineData("site/with/slashes")]
+    [InlineData("site with spaces")]
+    [InlineData("näme#!")]
+    public void Create_WithUnsafeSiteId_SanitizesAndSucceeds(string siteId)
+    {
+        var factory = new DefaultSiteClientFactory();
+        var client = factory.Create(Sys, siteId, Contacts()); // pre-fix: InvalidActorNameException
+        Assert.NotNull(client);
+    }
+
+    [Theory]
+    [InlineData("plant-01", "plant-01")]
+    [InlineData("a/b c", "a_b_c")]
+    [InlineData("", "site")]
+    public void SanitizeForActorName_ProducesValidPathElement(string input, string expected)
+    {
+        Assert.Equal(expected, DefaultSiteClientFactory.SanitizeForActorName(input));
+        Assert.True(ActorPath.IsValidPathElement(
+            DefaultSiteClientFactory.SanitizeForActorName(input)));
+    }
+}