diff --git a/src/MeshWeaver.Data.Contract/EntityStore.cs b/src/MeshWeaver.Data.Contract/EntityStore.cs index 294898538e..8625955e46 100644 --- a/src/MeshWeaver.Data.Contract/EntityStore.cs +++ b/src/MeshWeaver.Data.Contract/EntityStore.cs @@ -111,12 +111,34 @@ public virtual bool Equals(EntityStore? other) && value.Equals(x.Value)); } - /// Returns a hash code derived from the contained collections. - /// The hash code. + /// + /// Hash derived from the collection NAMES and their sizes — deliberately NOT from the + /// contained instances. + /// + /// 🚨 The previous version aggregated InstanceCollection.GetHashCode(), which walked + /// every stored instance; because a stored instance can transitively hold this very store + /// (EntityStore → ChangeItem → InstanceCollection → EntityStore), that recursion was + /// unbounded and killed pods with an uncatchable StackOverflowException (#2170/#2171). + /// + /// + /// It also threw ("Sequence contains no elements") for + /// an EMPTY store: Aggregate was called without a seed. An empty store now hashes + /// normally. + /// + /// + /// Contract-consistent with : equal stores hold the same + /// collection names, and equal s hold the same number of + /// instances, so equal stores hash equal. The XOR is order-independent. + /// + /// + /// A bounded, cycle-free hash code. public override int GetHashCode() - => Collections.Values - .Select(x => x.GetHashCode()) - .Aggregate((x, y) => x ^ y); + { + var hash = 0; + foreach (var kvp in Collections) + hash ^= HashCode.Combine(kvp.Key, kvp.Value.Instances.Count); + return HashCode.Combine(hash, Collections.Count); + } internal IEnumerable ComputeChanges(string collection, InstanceCollection updated) diff --git a/src/MeshWeaver.Data.Contract/InstanceCollection.cs b/src/MeshWeaver.Data.Contract/InstanceCollection.cs index d228fadb31..efaac651f8 100644 --- a/src/MeshWeaver.Data.Contract/InstanceCollection.cs +++ b/src/MeshWeaver.Data.Contract/InstanceCollection.cs @@ -167,8 +167,35 @@ public virtual bool Equals(InstanceCollection? other) ); } - /// Returns a hash code derived from the contained instances. - /// The hash code. - public override int GetHashCode() => - Instances.Values.Select(x => x.GetHashCode()).Aggregate(0, (x, y) => x ^ y); + /// + /// Hash derived from the instance KEYS and the count — deliberately NOT from the instance + /// VALUES. + /// + /// 🚨 holds ARBITRARY user objects, and in a running mesh those + /// objects reach back into the store that contains this collection + /// (InstanceCollection → EntityStore → ChangeItem → InstanceCollection). Hashing the + /// values therefore walked an unbounded object graph and died on an uncatchable + /// StackOverflowException, terminating the pod (#2170/#2171). It was also O(entire graph) on + /// a routing hot path. + /// + /// + /// Keys are safe by construction — already hashed every one of them to + /// store it — and they satisfy the hash/equals contract against the value-based + /// : equal collections hold the same keys in the same + /// number, hence the same hash. The XOR is order-independent, so it does not depend on + /// enumeration order. Note + /// is excluded because Equals ignores it — including it + /// would break the contract. + /// + /// + /// A bounded, cycle-free hash code. + public override int GetHashCode() + { + // foreach over the struct enumerator: no LINQ delegate/iterator allocation, unlike the + // Select+Aggregate this replaced. + var keyHash = 0; + foreach (var kvp in Instances) + keyHash ^= kvp.Key.GetHashCode(); + return HashCode.Combine(keyHash, Instances.Count); + } } diff --git a/src/MeshWeaver.Data/ChangeItem.cs b/src/MeshWeaver.Data/ChangeItem.cs index 2391d26671..e31941d0af 100644 --- a/src/MeshWeaver.Data/ChangeItem.cs +++ b/src/MeshWeaver.Data/ChangeItem.cs @@ -61,4 +61,25 @@ long Version : this(Value, null, StreamId, ChangeType.Full, Version, null) { } + /// + /// Hash derived from the change's own identity fields — deliberately NOT from + /// . + /// + /// 🚨 is the whole synchronized state (typically an + /// , or the stream state of an arbitrary ). + /// The record-synthesized hash walked it, which made ChangeItem the third layer of the + /// unbounded InstanceCollection → EntityStore → ChangeItem → InstanceCollection hash + /// recursion that killed pods with an uncatchable StackOverflowException (#2170/#2171), and + /// made a single hash O(the entire object graph) on a hot path. + /// + /// + /// The four fields used are all part of the synthesized Equals, so the hash/equals + /// contract holds: equal change items agree on them and therefore hash equal. Value-based + /// Equals is intentionally left untouched — the SetCurrent dedup that suppresses + /// re-emission of an identical re-synced store depends on it. + /// + /// + /// A bounded, cycle-free hash code. + public override int GetHashCode() => + HashCode.Combine(StreamId, ChangedBy, ChangeType, Version); } diff --git a/src/MeshWeaver.Data/Serialization/SynchronizationStream.cs b/src/MeshWeaver.Data/Serialization/SynchronizationStream.cs index f69be81e28..4ef3578802 100644 --- a/src/MeshWeaver.Data/Serialization/SynchronizationStream.cs +++ b/src/MeshWeaver.Data/Serialization/SynchronizationStream.cs @@ -3,6 +3,7 @@ using System.Reactive.Linq; using System.Reactive.Subjects; using System.Reflection; +using System.Runtime.CompilerServices; using System.Text.Json; using System.Text.Json.Serialization; using MeshWeaver.Messaging; @@ -1611,6 +1612,37 @@ private void RequestFreshSnapshot() internal StreamConfiguration Configuration { get; } + /// + /// 🚨 REFERENCE identity — deliberately NOT the record-synthesized structural equality. + /// + /// A stream is a LIVE object, not a value: it owns a , mutable + /// current state, a hosted hub, subscriptions and disposal state. Two distinct stream + /// instances are never "the same stream", so reference identity IS the correct semantics + /// (the synthesized version already behaved this way in practice — each instance carries its + /// own subject, compared by reference). + /// + /// + /// It is also the only SAFE semantics. The synthesized GetHashCode/Equals walk + /// every instance field, including — and + /// points straight back here. That closed a + /// cycle SynchronizationStream.GetHashCode → StreamConfiguration.GetHashCode → + /// EqualityComparer<ISynchronizationStream<TStream>>.Default.GetHashCode(Stream) → + /// … with no base case, so ANY hash of a stream instance (a dictionary key, a log scope, + /// a HashSet) recursed until the process died on an uncatchable StackOverflowException + /// (#2163/#2164/#2172/#2173/#2174/#2175 — repeated pod kills in prod). + /// + /// + /// The stream to compare against. + /// True only when is this very instance. + public virtual bool Equals(SynchronizationStream? other) => ReferenceEquals(this, other); + + /// + /// Reference-identity hash — see for why + /// structural hashing of a stream is both wrong and fatal. + /// + /// The runtime identity hash of this instance. + public override int GetHashCode() => RuntimeHelpers.GetHashCode(this); + /// /// Disposes the stream: completes the underlying store and tears down the hosted hub. @@ -1873,4 +1905,50 @@ public StreamConfiguration WithDeferredInitialization(bool deferred = t public StreamConfiguration WithDeferredInitialization( string gateName, Predicate allowDuringInit) => this with { DeferredInitialization = true, DeferredGateName = gateName, DeferredGatePredicate = allowDuringInit }; + + /// + /// 🚨 Compares this configuration's OWN values; the back-pointer + /// participates BY REFERENCE and is never traversed structurally. + /// + /// points back at the stream that OWNS this configuration + /// (new StreamConfiguration<TStream>(this)), so letting the record-synthesized + /// members walk it closes an unbounded cycle through + /// — the StackOverflow that killed portal pods + /// (#2163/#2164/#2172/#2173/#2174/#2175). The owning stream is part of this configuration's + /// IDENTITY, not of its value, which is exactly what reference comparison expresses. + /// + /// + /// The configuration to compare against. + /// True if both configurations belong to the same stream and carry the same settings. + public virtual bool Equals(StreamConfiguration? other) => + other is not null + && (ReferenceEquals(this, other) + || (ReferenceEquals(Stream, other.Stream) + && ClientId == other.ClientId + && Equals(Subscriber, other.Subscriber) + && NullReturn == other.NullReturn + && RunsAsInfrastructure == other.RunsAsInfrastructure + && DeferredInitialization == other.DeferredInitialization + && DeferredGateName == other.DeferredGateName + && Equals(Initialization, other.Initialization) + && Equals(ObservableInitialization, other.ObservableInitialization) + && Equals(ExceptionCallback, other.ExceptionCallback) + && Equals(DeferredGatePredicate, other.DeferredGatePredicate))); + + /// + /// Hash over this configuration's own settings plus the owning stream's REFERENCE identity — + /// never a structural walk of . See + /// . The delegate-valued settings are + /// deliberately omitted (equal configurations still hash equal, which is all the contract asks). + /// + /// A bounded, cycle-free hash code. + public override int GetHashCode() => + HashCode.Combine( + RuntimeHelpers.GetHashCode(Stream), + ClientId, + Subscriber, + NullReturn, + RunsAsInfrastructure, + DeferredInitialization, + DeferredGateName); } diff --git a/src/MeshWeaver.Data/Workspace.cs b/src/MeshWeaver.Data/Workspace.cs index 6fa94826e8..71cd4ac530 100644 --- a/src/MeshWeaver.Data/Workspace.cs +++ b/src/MeshWeaver.Data/Workspace.cs @@ -439,15 +439,16 @@ WorkspaceReference reference // workspace disposal would leak the stream's heartbeat for the process lifetime). // Keyed set (value unused) rather than a bag so a per-identity release can remove // matching entries without the drain-and-re-add race a ConcurrentBag would force. - // 🚨 MUST hash by REFERENCE: SynchronizationStream is a record whose generated - // GetHashCode recurses into StreamConfiguration (which holds the stream back) — - // value-hashing a stream instance stack-overflows the process. Stream identity - // here IS the instance, so reference semantics is also the correct semantics. + // Hashes by REFERENCE: stream identity here IS the instance. SynchronizationStream now + // declares reference identity itself (the generated structural GetHashCode recursed into + // StreamConfiguration, which holds the stream back — an uncatchable StackOverflow, #2163…), + // so this comparer is no longer load-bearing for that; it is kept because ISynchronizationStream + // is an interface and reference semantics must hold for ANY implementation put in this set. private readonly ConcurrentDictionary _evictedRemoteStreams = new(StreamReferenceComparer.Instance); /// Reference-identity comparer for stream instances — see the - /// field note (record GetHashCode recursion). + /// field note. private sealed class StreamReferenceComparer : IEqualityComparer { public static readonly StreamReferenceComparer Instance = new(); diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-25-hash-recursion-pod-crash.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-25-hash-recursion-pod-crash.md new file mode 100644 index 0000000000..ccdd2c2a4a --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-25-hash-recursion-pod-crash.md @@ -0,0 +1,22 @@ +--- +Name: Portal no longer crashes on a self-referencing data graph +Category: Fix +Description: Fixed an unbounded hash-code recursion that could kill a portal instance outright, dropping every open page on it. +Icon: Sparkle +Order: -20260825 +--- + +# Portal no longer crashes on a self-referencing data graph + +Two places in the data layer computed an object's hash code by walking everything it +contained. When the data pointed back at itself — which happens routinely once a +synchronization stream or an entity store is involved — that walk never finished and the +portal process died on the spot, taking every page open on that instance with it. + +Streams and their configuration now compare and hash by identity, which is what they always +meant: two live streams are two different streams, never two copies of the same value. Stores +and collections hash from their keys and sizes instead of from the arbitrary objects they +carry, so the cost no longer grows with the size of your workspace either. + +A related crash is fixed at the same time: an empty store used to throw when something asked +for its hash code. diff --git a/test/MeshWeaver.Data.Test/HashRecursionTest.cs b/test/MeshWeaver.Data.Test/HashRecursionTest.cs new file mode 100644 index 0000000000..bb6dea4d3a --- /dev/null +++ b/test/MeshWeaver.Data.Test/HashRecursionTest.cs @@ -0,0 +1,281 @@ +using System; +using System.Collections.Generic; +using MeshWeaver.Data.Serialization; +using MeshWeaver.Fixture; + +namespace MeshWeaver.Data.Test; + +/// +/// Pins the two unbounded GetHashCode cycles that killed portal pods with an uncatchable +/// StackOverflowException (#2163/#2164/#2172/#2173/#2174/#2175 and #2170/#2171). +/// +/// Cycle 1 — record-synthesized structural hashing over a back-reference. +/// SynchronizationStream<T> is a record, so the compiler synthesized a +/// GetHashCode walking every instance field — including Configuration. +/// StreamConfiguration<T>'s primary-ctor property Stream is a back-pointer to +/// the owning stream (new StreamConfiguration<T>(this)), so hashing a stream ran +/// SynchronizationStream.GetHashCode → StreamConfiguration.GetHashCode → +/// EqualityComparer<ISynchronizationStream<T>>.Default.GetHashCode(Stream) → … +/// forever — exactly the captured production stack. +/// +/// Cycle 2 — hand-written structural hashing over an arbitrary object graph. +/// InstanceCollection.GetHashCode aggregated the hashes of its VALUES (arbitrary user +/// objects), EntityStore.GetHashCode aggregated its collections, and +/// ChangeItem<T>'s synthesized hash walked its Value — so a value that +/// transitively holds the store closed the loop +/// InstanceCollection → EntityStore → ChangeItem → InstanceCollection. +/// +/// The fix is semantic, not a depth guard. A stream is a live identity object, so it +/// hashes by reference; a configuration hashes its own values and treats its owner as a reference; +/// stores and collections hash their KEYS and counts, never the instance values they carry. +/// +/// A StackOverflowException cannot be caught in .NET, so these tests never let the +/// recursion run free: is the cycle-closing value and it THROWS once +/// it has been re-entered, turning a pre-fix regression into a clean red test instead of a dead +/// test host. The assertion is stronger than "it terminated" — it is that the instance value was +/// never traversed at all. +/// +public class HashRecursionTest(ITestOutputHelper output) : HubTestBase(output) +{ + private const int ReentrancyBudget = 8; + + /// + /// A value stored inside an that points back at the graph + /// containing it — the shape a running mesh actually produces. Hashing it re-enters the graph, + /// so it counts its own invocations and refuses to recurse past + /// : pre-fix this surfaces as a deterministic exception naming + /// the defect, instead of a StackOverflow that takes the test host down with it. + /// + private sealed class ReentrancyProbe + { + /// The back-pointer that closes the cycle (a ChangeItem holding the store). + public object? Back { get; set; } + + /// How many times structural hashing has reached this instance value. + public int HashCalls { get; private set; } + + /// Reference identity — this probe measures hashing, not equality. + public override bool Equals(object? obj) => ReferenceEquals(this, obj); + + /// + public override int GetHashCode() + { + HashCalls++; + if (HashCalls > ReentrancyBudget) + throw new InvalidOperationException( + $"Instance-value hashing re-entered {HashCalls} times: the store/collection " + + "hash is walking its VALUES again and would StackOverflow in production " + + "(#2170/#2171)."); + return Back?.GetHashCode() ?? 0; + } + } + + /// + /// Builds the exact production cycle: an whose + /// holds a value that points back at a + /// carrying that same store. + /// + private static (EntityStore Store, InstanceCollection Collection, ChangeItem Change, ReentrancyProbe Probe) + BuildCyclicGraph() + { + var probe = new ReentrancyProbe(); + var collection = new InstanceCollection( + new Dictionary { ["id-1"] = probe, ["id-2"] = "a plain value" }); + var store = new EntityStore( + new Dictionary { ["Probes"] = collection }); + var change = new ChangeItem(store, "stream-1", 1); + probe.Back = change; // ← closes the loop + return (store, collection, change, probe); + } + + // --------------------------------------------------------------------------------------- + // Cycle 2 — InstanceCollection / EntityStore / ChangeItem + // --------------------------------------------------------------------------------------- + + /// + /// Hashing anywhere on the cyclic store graph terminates, and does so because the instance + /// VALUES are never traversed — the property that makes it bounded for ANY object graph, not + /// just this one. + /// + [HubFact] + public void CyclicStoreGraph_HashesWithoutTraversingInstanceValues() + { + var (store, collection, change, probe) = BuildCyclicGraph(); + + // Each of these was an entry point into the unbounded walk in production. + _ = collection.GetHashCode(); + _ = store.GetHashCode(); + _ = change.GetHashCode(); + + probe.HashCalls.Should().Be(0, + "InstanceCollection/EntityStore/ChangeItem must hash their KEYS and counts, never the " + + "arbitrary instance values — a value that reaches back into the store is exactly how " + + "the production StackOverflow (#2170/#2171) was reached"); + } + + /// + /// The hash must be stable, not merely terminating — a hash that varies between calls silently + /// corrupts every dictionary it is used in. + /// + [HubFact] + public void CyclicStoreGraph_HashIsStableAcrossCalls() + { + var (store, collection, change, _) = BuildCyclicGraph(); + + collection.GetHashCode().Should().Be(collection.GetHashCode()); + store.GetHashCode().Should().Be(store.GetHashCode()); + change.GetHashCode().Should().Be(change.GetHashCode()); + } + + /// + /// The seedless Aggregate((x, y) => x ^ y) in EntityStore.GetHashCode threw + /// ("Sequence contains no elements") for a store with + /// no collections — a latent crash on the emptiest possible input. + /// + [HubFact] + public void EmptyEntityStore_Hashes_WithoutThrowing() + { + Action hashEmptyStore = () => { _ = new EntityStore().GetHashCode(); }; + hashEmptyStore.Should().NotThrow( + "an empty store is a legal store; the hash used Aggregate with no seed"); + + Action hashEmptyCollection = () => { _ = new InstanceCollection().GetHashCode(); }; + hashEmptyCollection.Should().NotThrow(); + } + + /// + /// The hash/equals contract: equal objects hash equal. The hashes got WEAKER (keys and counts + /// rather than values) — weaker is fine, inconsistent is not. + /// + [HubFact] + public void EqualStoresAndCollections_HashEqual() + { + InstanceCollection Collection() => new( + new Dictionary { ["a"] = "one", ["b"] = "two" }); + EntityStore Store() => new( + new Dictionary { ["C"] = Collection() }); + + var c1 = Collection(); + var c2 = Collection(); + c1.Equals(c2).Should().BeTrue(); + c1.GetHashCode().Should().Be(c2.GetHashCode()); + + var s1 = Store(); + var s2 = Store(); + s1.Equals(s2).Should().BeTrue(); + s1.GetHashCode().Should().Be(s2.GetHashCode()); + + var ch1 = new ChangeItem(s1, "stream-1", 7); + var ch2 = new ChangeItem(s2, "stream-1", 7); + ch1.Equals(ch2).Should().BeTrue( + "value equality drives the SetCurrent dedup and is deliberately unchanged"); + ch1.GetHashCode().Should().Be(ch2.GetHashCode()); + } + + /// + /// The weakened hash still has to discriminate on the things it claims to hash — otherwise it + /// degenerates every dictionary into a linked list. + /// + [HubFact] + public void DifferentKeysOrSizes_HashDifferently() + { + var a = new InstanceCollection(new Dictionary { ["a"] = "x" }); + var b = new InstanceCollection(new Dictionary { ["b"] = "x" }); + var ab = new InstanceCollection( + new Dictionary { ["a"] = "x", ["b"] = "x" }); + + a.GetHashCode().Should().NotBe(b.GetHashCode(), "different keys"); + a.GetHashCode().Should().NotBe(ab.GetHashCode(), "different size"); + + var s1 = new EntityStore(new Dictionary { ["One"] = a }); + var s2 = new EntityStore(new Dictionary { ["Two"] = a }); + s1.GetHashCode().Should().NotBe(s2.GetHashCode(), "different collection names"); + } + + // --------------------------------------------------------------------------------------- + // Cycle 1 — SynchronizationStream / StreamConfiguration + // --------------------------------------------------------------------------------------- + + /// + /// A stream and its configuration hash without recursing through the configuration's + /// back-pointer to the stream — and the stream compares by REFERENCE, because two distinct + /// live streams (each owning its own ReplaySubject, hub and disposal state) are never the same + /// stream. + /// + [HubFact] + public void Stream_And_ItsConfiguration_HashByIdentity_NotStructurally() + { + var host = GetHost(); + using var stream = new SynchronizationStream( + new StreamIdentity(host.Address, null), + host, + new EntityReference("Probes", "id-1"), + new ReduceManager(host), + null); + + // Pre-fix each of these entered the unbounded stream ↔ configuration cycle + // and never returned. + var streamHash = stream.GetHashCode(); + var configHash = stream.Configuration.GetHashCode(); + + streamHash.Should().Be(stream.GetHashCode(), "an identity hash must be stable"); + configHash.Should().Be(stream.Configuration.GetHashCode()); + + // A configuration constructed independently over the same stream also terminates — + // the back-pointer is never traversed structurally, whoever holds it. + _ = new StreamConfiguration(stream).GetHashCode(); + + stream.Equals(stream).Should().BeTrue(); + } + + /// + /// Reference identity is the SEMANTICS, not just the mechanism: two streams built with the + /// same identity, host and reference are still two different live streams. + /// + [HubFact] + public void TwoDistinctStreams_AreNeverEqual() + { + var host = GetHost(); + using var a = new SynchronizationStream( + new StreamIdentity(host.Address, null), host, + new EntityReference("Probes", "id-1"), new ReduceManager(host), null); + using var b = new SynchronizationStream( + new StreamIdentity(host.Address, null), host, + new EntityReference("Probes", "id-1"), new ReduceManager(host), null); + + a.Equals(b).Should().BeFalse( + "a stream owns a ReplaySubject, mutable current state, a hosted hub and disposal state " + + "— distinct instances are distinct streams"); + (a == b).Should().BeFalse(); + } + + /// + /// StreamConfiguration keeps value semantics over its OWN settings (it is used as a + /// with-based builder throughout: WithClientId / WithSubscriber / AsInfrastructure / …), + /// while its owning stream participates by reference only. + /// + [HubFact] + public void StreamConfiguration_KeepsValueSemantics_OverItsOwnSettings() + { + var host = GetHost(); + using var stream = new SynchronizationStream( + new StreamIdentity(host.Address, null), host, + new EntityReference("Probes", "id-1"), new ReduceManager(host), null); + + // Derive both from the same base so the delegate-valued settings are literally shared — + // the difference under test is the client id, nothing else. + var baseConfig = new StreamConfiguration(stream); + var sameA = baseConfig.WithClientId("client-a"); + var sameB = baseConfig.WithClientId("client-a"); + var other = baseConfig.WithClientId("client-b"); + + sameA.Equals(sameB).Should().BeTrue( + "the settings are identical and the owner is the same stream"); + sameA.GetHashCode().Should().Be(sameB.GetHashCode()); + sameA.Equals(other).Should().BeFalse("the client id differs"); + sameA.GetHashCode().Should().NotBe(other.GetHashCode()); + + baseConfig.AsInfrastructure().Equals(baseConfig).Should().BeFalse(); + baseConfig.ReturnNullWhenNotPresent().Equals(baseConfig).Should().BeFalse(); + } +}