Repository navigation
Milestone 3.5: widen system/address UID int32 -> int64 (Artery prerequisite) - #8317
Aaronontheweb merged 9 commits into
Conversation
- AddressUid.Uid + AddressUidExtension.Uid(ActorSystem) widened to long; default generation unchanged (ThreadLocalRandom int-range, rolling-upgrade safe) - new akka.remote.use-64bit-system-uids switch (default off) enables full-range nonzero 64-bit uid generation via netstandard2.0-safe byte-based RNG - HandshakeInfo/refuseUid, EndpointRegistry, Endpoint.cs (HopelessAssociation, ReliableDeliverySupervisor, GotUid, EndpointWriter/Reader), EndpointManager messages (Pass/Quarantined/Quarantine/ResendState) re-typed long/long? - RemoteWatcher HeartbeatRsp/_addressUids/Quarantine re-typed long - quarantine API hard re-type: IRemoteActorRefProvider/RemoteActorRefProvider/ RemoteTransport/Remoting.Quarantine(Address, long?); QuarantinedEvent(Address, long) - MiscMessageSerializer + AkkaPduCodec 32-bit narrowing casts removed (wire already 64-bit: ContainerFormats uint64, WireFormats fixed64) Akka.Cluster intentionally left broken until the next task group commit.
…izer-v2 schema (Decision 11)
…g system uid - UniqueAddress(Address, long uid) + long Uid; GetHashCode via Uid.GetHashCode() (value-identical to the old int hash for legacy-range uids); CompareTo/Equals already long-safe - ClusterMessages.proto UniqueAddress.uid uint32 -> uint64 (same varint wire type; binary-compatible for uids <= uint32.MaxValue; regenerated by Grpc.Tools) - ClusterMessageSerializer: (uint)/(int) narrowing replaced with lossless (ulong)/(long) reinterpret casts at the proto boundary - DData SerializationSupport.UniqueAddressFromProto: (int) narrowing removed (ReplicatorMessages.proto already int64) - vclock node name verified string-only (ClusterDaemon.VclockName) - unchanged Whole solution builds green.
…hard break ApproveRemote + ApproveCluster (.DotNet and .Net variants): the ~10 inventoried members re-typed int->long, nothing else. All 18 API approval tests pass; dotnet build -warnaserror clean on the full solution.
… ledger entry - UniqueAddressWireCompatSpec: v1.5<->v1.6 gossip wire simulation - v1.6-emitted uint64 uid field read as uint32 (no truncation for uids <= uint32.MaxValue) and legacy uint32 bytes parsed by the widened v1.6 parser, plus serializer-level round-trips incl. >32-bit uids - AddressUidExtensionSpecs: default generation stays in [0, int.MaxValue]; use-64bit-system-uids = on yields nonzero uids; 64-draw statistical check that full-range generation escapes the legacy int range - >32-bit uid round-trips: MiscMessageSerializer (HeartbeatRsp), AkkaPduCodec (Associate handshake PDU), DData SerializationSupport (UniqueAddress) - BREAKING_CHANGES_V1.6.md: API + Wire rows for the int->long hard break (status Planned; flip to Merged with the PR link)
CI (build 128643) failed 5 multi-node tests in Akka.Remote.Tests.MultiNode: the Subject actors reply with AddressUidExtension.Uid(...) - now long - but the driver-side helpers still asserted ExpectMsg<int>/(int, IActorRef). Tell(object) erases the static type, so this compiled clean and only failed the runtime type check, on every run. Node2's port-bind failure in RemoteQuarantinePiercingSpec was collateral from the aborted barrier flow. Widened the ExpectMsg type params and uid locals to long in RemoteRestartedQuarantinedSpec, RemoteQuarantinePiercingSpec, and PiercingShouldKeepQuarantineSpec. No production change. All three specs pass locally through the MultiNode test adapter (6/6, net10.0 Release).
Follow-up to 12b8146: the three quarantine specs still used blocking TestKit/MNTR calls. Converted per repo policy (no sync TestKit methods, no .Wait()/Thread.Sleep in tests): - ExpectMsg/ExpectNoMsg -> ExpectMsgAsync/ExpectNoMsgAsync - EnterBarrier -> EnterBarrierAsync; RunOn -> RunOnAsync - Within/AwaitAssert -> WithinAsync/AwaitAssertAsync - TestConductor.Shutdown(...).Wait(...) -> await TestConductor.ShutdownAsync(...) - WhenTerminated.Wait(...) -> await WhenTerminated.WaitAsync(...) - Thread.Sleep -> await Task.Delay - _identifyWithUid Func now Task-returning with async lambda No changes to test logic, assertions, barrier names, or timeouts. Clean -warnaserror rebuild; all three specs pass locally via the MultiNode adapter (6/6, net10.0 Release).
Aaronontheweb
left a comment
There was a problem hiding this comment.
This widens the system/address UID from int to long end-to-end (Remote state machine, Cluster gossip, DData, and the quarantine/heartbeat/handshake APIs), as the prerequisite for Artery's 64-bit origin uid. Most of the diff is a mechanical int->long sweep with no wire change, since the handshake, heartbeat, and DData proto fields were already 64-bit. The risk concentrates in one line: ClusterMessages.proto field 2 going uint32 to uint64, byte-identical only while uids stay at or below uint32.MaxValue, which is why full-range generation is gated behind use-64bit-system-uids = off. Suggested reading order: the proto and the Remote.conf comment, then UniqueAddressWireCompatSpec, then skim the rest of the sweep.
| message UniqueAddress { | ||
| Akka.Remote.Serialization.Proto.Msg.AddressData address = 1; | ||
| uint32 uid = 2; | ||
| uint64 uid = 2; |
There was a problem hiding this comment.
Most wire-relevant line in the PR. Same field number (2) and same varint wire type as the old uint32, so the encoded bytes are identical for any uid at or below uint32.MaxValue. protobuf treats uint32/uint64 as compatible for exactly this reason. The only lossy case is a v1.6 node emitting a uid above uint32.MaxValue to a not-yet-upgraded v1.5 reader, whose generated ReadUInt32 keeps the low 32 bits. That path can't happen unless someone sets use-64bit-system-uids = on (default off); default generation stays in [0, int.MaxValue]. UniqueAddressWireCompatSpec pins the in-range behavior in both directions.
There was a problem hiding this comment.
TL;DR; so long as 64 bit UIDs aren't enabled, this will still be backwards compatible due to protobuf's treatment of 32 bit integers.
| # When enabled, system/address UIDs are generated across the full 64-bit range | ||
| # (nonzero, may be negative) instead of the legacy [0, int.MaxValue] range. | ||
| # DO NOT enable until EVERY node in the cluster and all remote peers run Akka.NET v1.6+: | ||
| # pre-v1.6 nodes silently truncate UIDs above uint32.MaxValue in cluster gossip. | ||
| use-64bit-system-uids = off |
There was a problem hiding this comment.
Opt-in on purpose. Safe to turn on only once every node and every remote peer runs v1.6+, because a pre-v1.6 reader truncates any gossip uid above uint32.MaxValue. Nothing enforces that precondition at runtime, so it stays an operator decision, same as other rolling-upgrade flags. If reviewers would rather hard-gate it, this is the spot to raise that. A cold restart regenerates the uid regardless, which is the path Artery adoption takes anyway, so the flag never has to flip mid-cluster.
| /// </param> | ||
| internal AddressUid(bool use64BitUid) | ||
| { | ||
| Uid = use64BitUid ? Generate64BitUid() : ThreadLocalRandom.Current.Next(); |
There was a problem hiding this comment.
Default branch (use64BitUid == false) is the exact pre-existing code, ThreadLocalRandom.Current.Next(), so nothing changes for anyone who doesn't opt in. Generate64BitUid() (below) uses NextBytes + BitConverter.ToInt64 because Random.NextInt64() doesn't exist on netstandard2.0/net48. It retries on 0 since 0 is reserved as a sentinel, and negative values are fine: the uid only ends up in the vclock node-name string (ClusterDaemon.VclockName, address + "-" + uid), which is never parsed back, so the sign is cosmetic.
| public override int GetHashCode() | ||
| { | ||
| return Uid; | ||
| return Uid.GetHashCode(); |
There was a problem hiding this comment.
long.GetHashCode() is (int)(uid ^ (uid >> 32)). For a uid in [0, int.MaxValue] the top 32 bits are 0, so this returns (int)uid, the same value the old return Uid; produced. Hash bucketing is unchanged for int-range uids, so a mixed-version cluster sees identical buckets. Equals (line 500, ==) and CompareTo (line 531, Uid.CompareTo) both widen cleanly. Hash codes aren't serialized, so there's no wire angle here.
| var message = new Proto.Msg.UniqueAddress(); | ||
| message.Address = AddressToProto(uniqueAddress.Address); | ||
| message.Uid = (uint)uniqueAddress.Uid; | ||
| message.Uid = (ulong)uniqueAddress.Uid; |
There was a problem hiding this comment.
(ulong) on write here and (long) on read at line 588 are reinterpret casts, lossless in both directions including negative uids. The gossip proto field is uint64, so the round-trip is bit-exact. No wire change lives in this method; the only thing that went away is the C# narrowing to int.
| { | ||
| Sys.ActorSelection(Node(role) / "user" / actorName).Tell("identify"); | ||
| return ExpectMsg<(int, IActorRef)>(); | ||
| return await ExpectMsgAsync<(long, IActorRef)>(); |
There was a problem hiding this comment.
This is the break the multi-node run caught. Once the Subject actor started replying with (long, IActorRef), this matcher still asked for (int, IActorRef). Tell(object) boxes the tuple and erases the static type, so it compiled clean and only missed the runtime tuple-type check, which dropped the message. That's why the unit suites can't catch this class of regression: the boxing hides it, and only the MNTR specs drive a real cross-process send/receive. Same one-line retype in RemoteRestartedQuarantinedSpec and PiercingShouldKeepQuarantineSpec.
|
|
||
| [MultiNodeFact] | ||
| public void PiercingShouldKeepQuarantineSpecs() | ||
| public async Task PiercingShouldKeepQuarantineSpecs() |
There was a problem hiding this comment.
The async rewrite in this file is mechanical, done to satisfy the repo test policy (no sync TestKit calls, no .Wait() / Thread.Sleep). RunOn becomes RunOnAsync, ExpectNoMsg becomes ExpectNoMsgAsync, Thread.Sleep(1000) becomes Task.Delay(1000). Barriers, timeouts, and assertions are untouched, so it's the same behavior run through async plumbing.
| /// effectively impossible unless the generator is broken (e.g. silently clamped back to int range). | ||
| /// </summary> | ||
| [Fact] | ||
| public void AddressUid_64bit_generation_should_draw_at_least_one_uid_outside_the_int_range() |
There was a problem hiding this comment.
Each draw falls back into [0, int.MaxValue] with p ~= 2^31 / 2^64 ~= 1.2e-10, so across 64 draws the Assert.Contains(... uid outside int range) is deterministic for any working generator. The point is to catch Generate64BitUid getting silently clamped back to int range, which is the plausible way a later cleanup breaks full-range generation. The NotEqual(0) assertion guards the sentinel retry loop.
| public AddressUidExtension() { } | ||
| public override Akka.Remote.AddressUid CreateExtension(Akka.Actor.ExtendedActorSystem system) { } | ||
| public static int Uid(Akka.Actor.ActorSystem system) { } | ||
| public static long Uid(Akka.Actor.ActorSystem system) { } |
There was a problem hiding this comment.
Fastest way to check the public-surface blast radius. The approved-file delta is only the uid members: AddressUid.Uid, AddressUidExtension.Uid, the three Quarantine(Address, long?) overloads plus RemoteWatcher.Quarantine, QuarantinedEvent ctor/Uid, HeartbeatRsp ctor/AddressUid, and on the Cluster side UniqueAddress ctor/Uid. Nothing else shifted, so no unrelated API drift rode along. All four verified files (DotNet/Net x Remote/Cluster) carry the same int->long delta.
| public UniqueAddress UniqueAddressFromProto(Proto.Msg.UniqueAddress address) | ||
| { | ||
| return new UniqueAddress(AddressFromProto(address.Address), (int)address.Uid); | ||
| return new UniqueAddress(AddressFromProto(address.Address), address.Uid); |
There was a problem hiding this comment.
DData's UniqueAddress.uid in ReplicatorMessages.proto was already int64 before this change, so this is just the C# (int) narrowing coming off. UniqueAddressSerializationSupportSpec adds a >32-bit round-trip over UniqueAddressToProto / UniqueAddressFromProto to lock that in.
- BREAKING_CHANGES_V1.6.md: flip the two #8317 rows Planned -> Merged - archive the completed OpenSpec change to openspec/changes/archive/2026-07-04-widen-system-uid-to-64bit - IMPLEMENTATION_ORDER.md: mark Milestone 3.5 DONE, point at archived change
Implements the
widen-system-uid-to-64bitOpenSpec change — Milestone 3.5 of the v1.6 plan (IMPLEMENTATION_ORDER.md), sequenced beforeartery-tcp-remoting(the Artery frame header carries a 64-bit origin UID).What changed
Hard API re-type
int→long(Decision 1 — no overloads, no[Obsolete]; v1.6 is a breaking cycle and theUidfields/props can't be overloaded anyway):AddressUid.Uid,AddressUidExtension.Uid(ActorSystem)UniqueAddress(Address, long)/.Uid(cluster member identity + gossip vclock)QuarantinedEvent(Address, long)/.UidRemoteWatcher.HeartbeatRsp(long)/.AddressUidQuarantine(Address, long? uid)acrossIRemoteActorRefProvider/RemoteActorRefProvider/RemoteTransport/Remoting/RemoteWatcherHandshakeInfo+ refuseUid path,EndpointRegistry,Endpoint.cs,EndpointManagermessages,ClusterDaemonWire (Decision 3 — minimal): only
ClusterMessages.protoUniqueAddress.uidwidensuint32 → uint64. Same varint wire type, so values ≤uint32.MaxValueare binary-compatible with v1.5 readers. Handshake (fixed64), RemoteWatcher heartbeat (uint64), and DData (int64) were already 64-bit on the wire — only the C# narrowing casts were removed.Rolling-upgrade safety (Decision 2 — value gating): default UID generation is unchanged (
ThreadLocalRandom,[0, int.MaxValue]). Full-range nonzero 64-bit generation is opt-in via the newakka.remote.use-64bit-system-uids = on(netstandard2.0-safe RNG), documented as requiring every node/peer on v1.6+ first.UniqueAddress.GetHashCode()is value-identical to the old int hash for legacy-range uids, and the vclock node name is string-only — mixed-version gossip is unaffected.Verification
UniqueAddressWireCompatSpec: simulates both sides of the v1.5↔v1.6 gossip boundary at the protobuf level (v1.6-emitteduint64field read asuint32without truncation; legacyuint32bytes parsed by the widened parser)HeartbeatRsp,Associatehandshake PDU, DDataSerializationSupport-warnaserrorBREAKING_CHANGES_V1.6.md: API + Wire rows added in this PR, per the v1.6 ledger policyCoordination
#12345), alreadylong