fix(cluster): ClusterRoleInfo._roleLeaders stays Ordinal — Akka tracks role-leadership per exact gossiped role string (review finding, R3-04)

2026-08-20 10:52:51 -04:00
parent 027c423502
commit 3648d3c129
2 changed files with 144 additions and 6 deletions
@@ -0,0 +1,129 @@
+using System.Net;
+using System.Net.Sockets;
+using Akka.Hosting;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Options;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.Interfaces;
+using ZB.MOM.WW.OtOpcUa.Commons.Types;
+
+namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests;
+
+/// <summary>
+///     Review finding on R3-04 (Important, 2026-08-20): <c>_roleLeaders</c> must stay
+///     <see cref="StringComparer.Ordinal"/>, NOT <see cref="StringComparer.OrdinalIgnoreCase"/> like
+///     <c>_membersByRole</c>. Akka raises a SEPARATE <c>RoleLeaderChanged</c> event per exact gossiped
+///     role string — "driver" and "Driver" are two entirely distinct roles to Akka's own role-leader
+///     computation, each with its own (possibly different) leader member. An ignore-case dictionary
+///     would collapse both onto ONE key, and whichever role-leader-changed event landed LAST would
+///     silently overwrite the other — <c>RoleLeader("driver")</c> could then return the "Driver"
+///     role's leader instead of (or in addition to losing) the "driver" role's own.
+/// </summary>
+/// <remarks>
+///     <c>_membersByRole</c> stays <see cref="StringComparer.OrdinalIgnoreCase"/> deliberately — it
+///     answers "give me every member carrying (a spelling of) this role", where merging casings is
+///     the whole point (archreview 01/C-9, rolling-upgrade peer defence in depth). <c>_roleLeaders</c>
+///     answers a per-EXACT-role-string question Akka itself keeps separate, so merging casings there
+///     loses information instead.
+/// </remarks>
+public sealed class ClusterRoleInfoRoleLeaderCaseTests
+{
+    private static int FreePort()
+    {
+        var listener = new TcpListener(IPAddress.Loopback, 0);
+        listener.Start();
+        var port = ((IPEndPoint)listener.LocalEndpoint).Port;
+        listener.Stop();
+        return port;
+    }
+
+    private const string SystemName = "clusterroleinfo-leader-case-test";
+
+    private static async Task<(IHost Host, IClusterRoleInfo RoleInfo)> StartNodeAsync(
+        int selfPort, int? peerPort, string role)
+    {
+        var self = $"akka.tcp://{SystemName}@127.0.0.1:{selfPort}";
+        var seeds = peerPort is null
+            ? new[] { self }
+            : new[] { self, $"akka.tcp://{SystemName}@127.0.0.1:{peerPort}" };
+
+        var options = new AkkaClusterOptions
+        {
+            SystemName = SystemName,
+            Hostname = "127.0.0.1",
+            PublicHostname = "127.0.0.1",
+            Port = selfPort,
+            // Deliberately ONE role each, spelled differently between the two nodes — that mismatch
+            // IS the scenario under test, not an oversight.
+            Roles = new[] { role },
+            SeedNodes = seeds,
+        };
+
+        var builder = Host.CreateDefaultBuilder();
+        builder.ConfigureServices(services =>
+        {
+            services.AddSingleton<IOptions<AkkaClusterOptions>>(Options.Create(options));
+            services.AddAkka(SystemName, (ab, sp) => ab.WithOtOpcUaClusterBootstrap(sp));
+            services.AddSingleton<IClusterRoleInfo, ClusterRoleInfo>();
+        });
+
+        var host = builder.Build();
+        await host.StartAsync();
+        return (host, host.Services.GetRequiredService<IClusterRoleInfo>());
+    }
+
+    /// <summary>
+    ///     Two single-member "roles" — "driver" on node A, "Driver" on node B, both Up in the SAME
+    ///     cluster. Each is the sole (and therefore leader) member of its own exact-string role. If
+    ///     <c>_roleLeaders</c> were case-insensitive, the two RoleLeaderChanged events would collide
+    ///     onto one dictionary slot and one of the two role-leader facts would be lost or wrong.
+    /// </summary>
+    [Fact]
+    public async Task RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries()
+    {
+        var portA = FreePort();
+        var portB = FreePort();
+
+        var (hostA, roleInfoA) = await StartNodeAsync(portA, peerPort: null, role: "driver");
+        try
+        {
+            var (hostB, _) = await StartNodeAsync(portB, peerPort: portA, role: "Driver");
+            try
+            {
+                var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(30);
+                NodeId? driverLeader = null;
+                NodeId? capitalDriverLeader = null;
+                while (DateTime.UtcNow < deadline)
+                {
+                    driverLeader = roleInfoA.RoleLeader("driver");
+                    capitalDriverLeader = roleInfoA.RoleLeader("Driver");
+                    if (driverLeader is not null && capitalDriverLeader is not null) break;
+                    await Task.Delay(200, TestContext.Current.CancellationToken);
+                }
+
+                driverLeader.ShouldNotBeNull("node A's own 'driver' role must elect itself leader");
+                capitalDriverLeader.ShouldNotBeNull(
+                    "node B's 'Driver' role must ALSO be tracked — a case-insensitive dictionary "
+                    + "would either lose this entry or clobber the 'driver' one with it");
+                driverLeader!.Value.ShouldNotBe(
+                    capitalDriverLeader!.Value,
+                    "the two roles are DISTINCT to Akka and must resolve to their own (different) "
+                    + "member — a merged dictionary slot would make these equal");
+                driverLeader.Value.ToString().ShouldContain(portA.ToString());
+                capitalDriverLeader.Value.ToString().ShouldContain(portB.ToString());
+            }
+            finally
+            {
+                await hostB.StopAsync(TestContext.Current.CancellationToken);
+                hostB.Dispose();
+            }
+        }
+        finally
+        {
+            await hostA.StopAsync(TestContext.Current.CancellationToken);
+            hostA.Dispose();
+        }
+    }
+}