diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs index 74151f43..689e435b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs @@ -23,12 +23,21 @@ public sealed class ClusterRoleInfo : IClusterRoleInfo, IDisposable private readonly string? _clusterRole; private readonly string? _clusterId; private readonly object _lock = new(); - // R3-04 (01/C-9, census row 17): OrdinalIgnoreCase — Akka compares gossiped role names ordinally, - // so a PEER running an older build can still gossip a miscased plain role during a rolling - // upgrade. Keying these by the exact-cased role string would leave MembersWithRole("driver") and - // RoleLeader("driver") blind to a peer that gossips "Driver", the same class of gap #532 closed - // for _localRoles above. - private readonly Dictionary _roleLeaders = new(StringComparer.OrdinalIgnoreCase); + // R3-04 (01/C-9, census row 17) — the two dictionaries below have DIFFERENT correct comparers, + // and that split is deliberate, not an oversight (review finding, 2026-08-20). + // + // _membersByRole answers "every member carrying (a spelling of) this role" — OrdinalIgnoreCase, + // so a PEER running an older build that still gossips a miscased plain role during a rolling + // upgrade is not invisible to MembersWithRole("driver"). Merging casings IS the point here. + // + // _roleLeaders must stay Ordinal. Akka computes — and raises RoleLeaderChanged for — role + // leadership PER EXACT GOSSIPED ROLE STRING: "driver" and "Driver" are two entirely distinct + // roles to Akka's own role-leader machinery, each independently electing its own leader. An + // OrdinalIgnoreCase dictionary here would collapse both onto ONE key, so whichever + // RoleLeaderChanged event landed last would silently overwrite the other's leader — turning two + // real, independent facts Akka tracks separately into one, wrong, merged one. Pinned by + // ClusterRoleInfoRoleLeaderCaseTests.RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries. + private readonly Dictionary _roleLeaders = new(StringComparer.Ordinal); private readonly Dictionary> _membersByRole = new(StringComparer.OrdinalIgnoreCase); private IActorRef? _subscriber; diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs new file mode 100644 index 00000000..82c3aee0 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs @@ -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; + +/// +/// Review finding on R3-04 (Important, 2026-08-20): _roleLeaders must stay +/// , NOT like +/// _membersByRole. Akka raises a SEPARATE RoleLeaderChanged 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 — RoleLeader("driver") could then return the "Driver" +/// role's leader instead of (or in addition to losing) the "driver" role's own. +/// +/// +/// _membersByRole stays 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). _roleLeaders +/// answers a per-EXACT-role-string question Akka itself keeps separate, so merging casings there +/// loses information instead. +/// +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>(Options.Create(options)); + services.AddAkka(SystemName, (ab, sp) => ab.WithOtOpcUaClusterBootstrap(sp)); + services.AddSingleton(); + }); + + var host = builder.Build(); + await host.StartAsync(); + return (host, host.Services.GetRequiredService()); + } + + /// + /// 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 + /// _roleLeaders 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. + /// + [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(); + } + } +}