Fix two unbounded GetHashCode recursions that StackOverflow the pod by rbuergi · Pull Request #2239 · Systemorph/MeshWeaver

rbuergi · 2026-08-25T17:50:41Z

Closes #2163, #2164, #2170, #2171, #2172, #2173, #2174, #2175 — eight incident issues, four
fingerprints, two root causes, one change.

The two cycles

Both are semantic defects — structural hashing applied to something that was never a value —
so both are fixed by giving the type the equality semantics it actually has. No depth limits, no
visited-sets, no guards (AGENTS.md: no band-aids).

Cycle 1 — record-synthesized structural hashing over a back-reference
(#2163/#2164/#2172/#2173/#2174/#2175, fingerprints 6e643370636ed34a / 52c67215dc2ff138 /
e089a7d2379666fb)

SynchronizationStream<T> is a record, so the compiler synthesized a GetHashCode/Equals
walking every instance field — including internal StreamConfiguration<T> Configuration.
StreamConfiguration<T>'s primary-ctor property Stream is a back-pointer to the owning stream
(new StreamConfiguration<TStream>(this), SynchronizationStream.cs:1042). So hashing a stream ran:

SynchronizationStream.GetHashCode
  → StreamConfiguration.GetHashCode
    → EqualityComparer<ISynchronizationStream<T>>.Default.GetHashCode(Stream)
      → SynchronizationStream.GetHashCode → …

with no base case — exactly the captured production stack. Any hash of a stream instance (a
dictionary key, a log scope, a HashSet) killed the process.

Cycle 2 — hand-written structural hashing over an arbitrary object graph (#2170/#2171,
fingerprint 786f05aa5de0c166)

InstanceCollection.GetHashCode aggregated the hashes of its values — an
ImmutableDictionary<object, object> of arbitrary user instances — EntityStore.GetHashCode
aggregated its collections, and ChangeItem<T>'s synthesized hash walked its Value. A stored
instance that reaches back into the store closes the loop
InstanceCollection → EntityStore → ChangeItem → InstanceCollection, which is the recorded stack.
It was also O(the entire object graph) on a routing hot path.

Why reference identity on the stream is the ROOT fix, not half of one

The two cycles are not independent: cycle 1 fed cycle 2. SynchronizationStream's
record-synthesized GetHashCode walks every instance field, including its private
ChangeItem<TStream>? current — so hashing a stream descended
SynchronizationStream → ChangeItem → EntityStore → InstanceCollection and, from there, into
whatever arbitrary instances the store happened to hold. Both recorded production stacks were
entered by hashing a STREAM, whether via the _evictedRemoteStreams dictionary (before its
hand-rolled reference-comparer workaround) or a log scope.

That is why giving the stream reference identity is the root fix rather than a partial one: it
removes the entry point. The container fixes below then close the same hole for any other caller
that reaches those types directly — belt and braces, deliberately, because
ISynchronizationStream is an interface and a future implementation must not be able to
reintroduce the descent.

The fix

SynchronizationStream<T> — reference identity (ReferenceEquals / RuntimeHelpers.GetHashCode).
A stream is a live object: it owns a ReplaySubject, mutable current state, a hosted hub, and
disposal state. Two distinct streams are never the same stream. This is not a behaviour change:
the synthesized Equals already compared the per-instance Store subject by reference, so
distinct instances were never equal — it just could not compute a hash without dying.
StreamConfiguration<T> — compares/hashes its own settings; the Stream back-pointer
participates by reference and is never traversed. Still a record; the with-based builder
API (WithClientId / WithSubscriber / AsInfrastructure / WithInitialization / …) is untouched.
InstanceCollection / EntityStore / ChangeItem<T> — hash from keys, collection names,
and counts. Bounded, cycle-free, allocation-free (foreach over the struct enumerator rather than
Select + Aggregate), and no longer proportional to the workspace size.

Bonus latent crash, fixed here too: EntityStore.GetHashCode called .Aggregate((x, y) => x ^ y)
with no seed, so an EMPTY store threw InvalidOperationException("Sequence contains no elements").

Every value-based `Equals` is deliberately left intact

The SetCurrent dedup that suppresses re-emission of an identical re-synced store depends on
value equality (MeshNodeContentEqualityTest.EntityStoreChain_WithJsonElementNodes_DedupesViaValueEquality
— its comment calls re-emission "the amplifier"). Weakening it to match the weakened hash would be
a real regression, and it is not needed: the hash/equals contract only requires that equal objects
hash equal, which each new hash satisfies (equal collections hold the same keys in the same
number; equal stores hold the same collection names with the same sizes; equal change items agree
on StreamId/ChangedBy/ChangeType/Version).

One caveat worth recording rather than papering over: InstanceCollection.Equals can itself
recurse on a cyclic graph, because it compares arbitrary values. It needs two distinct parallel
cyclic graphs to do so (record Equals short-circuits on ReferenceEquals, so a graph compared
with itself terminates), it has never appeared in any production trace, and every captured stack is
GetHashCode. Fixing it would mean breaking the dedup contract above, so it is left as-is and noted.

Verification — the tests are red before the fix, by measurement

test/MeshWeaver.Data.Test/HashRecursionTest.cs (8 tests) builds the real cyclic graphs. A
StackOverflowException cannot be caught in .NET, so the cycle-closing value is a probe that throws
once re-entered — a pre-fix regression is a clean red test, not a dead test host — and the assertion
is stronger than "it terminated": the instance values are never traversed at all.

Reverse-applied the fix and re-ran to confirm the tests actually fail without it:

test	without the fix
`CyclicStoreGraph_HashesWithoutTraversingInstanceValues`	`InvalidOperationException: Instance-value hashing re-entered 9 times…`
`EmptyEntityStore_Hashes_WithoutThrowing`	`InvalidOperationException: Sequence contains no elements`
`Stream_And_ItsConfiguration_HashByIdentity_NotStructurally`	test host crashed, exit code 134 (SIGABRT) — the StackOverflow itself

All 8 pass with the fix.

Independent corroboration that the diagnosis is right: Workspace.cs already carried a
hand-rolled StreamReferenceComparer whose comment reads "value-hashing a stream instance
stack-overflows the process" — a previous session hit this and worked around it locally. That
comment is updated (the comparer is kept, since ISynchronizationStream is an interface and
reference semantics must hold for any implementation put in that set).

Builds and tests

-c Release -warnaserror, 0 Error(s) / 0 Warning(s) on each: MeshWeaver.Data.Contract,
MeshWeaver.Data, MeshWeaver.Mesh.Contract, MeshWeaver.Graph, MeshWeaver.Blazor,
MeshWeaver.Hosting.Orleans, MeshWeaver.AI, MeshWeaver.Data.Test, MeshWeaver.Layout.Test,
MeshWeaver.Hosting.Monolith.Test, MeshWeaver.Documentation.Test.

MeshWeaver.Data.Test — 385/385 pass
MeshWeaver.Layout.Test — 474/474 pass (equality/hash tests included)
MeshWeaver.Documentation.Test WhatsNewEntryIntegrityTest — pass

No production code derives change detection from these hashes (swept every GetHashCode() call
site in src/); the only consumers are hash containers, where a weaker-but-consistent hash costs
distribution, never correctness.

What's New entry: Category: Fix — a crash a user notices as the page dropping under them.

🤖 Generated with Claude Code

CI note: the first run's two reds, diagnosed

The first run of this PR went red on two jobs. Both were investigated to a cause before the failed
jobs were re-run; recording it here so the next person who meets either does not repeat the work.

1. `Run tests (shard 3)` — `MeshGrpcTrustedKestrelTest` — environmental, proven

From the shard-3 .trx artifact:

=== TEST FAILED: Failed to bind to address http://127.0.0.1:39933: address already in use.
   at Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.SocketConnectionListener.Bind()
   at MeshGrpcTrustedKestrelTest…(MeshGrpcTrustedKestrelTest.cs:line 79)

A port collision on the runner — it threw at Bind() before doing any work, which is why it took
0 s. Shard 3 ran 2264 tests with exactly this 1 failure. Passes locally 1/1 both with and
without this diff.

2. `Plugins compile against this PR` — `Store/Catalog` — no mechanism found; considered and excluded

❌ StoreCatalogTests timed out waiting for: the recovered read lands — a timeout on a 20 ms-poll
WaitUntil, not an assertion mismatch. Three hypotheses were raised and each was disproved by
measurement, not by argument:

"A weakened hash broke change detection." That pipeline's dedup is
.DistinctUntilChanged(RegistryPackages.CatalogKey) (StoreManifestSource.cs:298), and
CatalogKey (RegistryPackages.cs:229) is a pure string — id@version, ordered. Across the
transition under test the keys are "Hosting@" → "Chess@|Hosting@": different strings, so
DistinctUntilChanged must emit. No hash, no container type, on that path. Sweeps for a
hash-based consumer came back empty in all three trees: MeshWeaver.Plugins (one hit, an
unrelated entity.GetHashCode() in ImportManager), content + samples/*/Data (zero), and
src/ (zero).
"The plugins tree moved underneath this run." It did not: this gate and the sibling PR's gate
both checked out plugins a886396c, and the sibling reported
ok Store/Catalog: compile=Ok render=ok tests=ok.
"The hash got much cheaper, and a large timing change can flip a latent race." The sharpest of
the three, and the reason this section exists — a change that alters only cost can still flip a
race, so "it only affects performance" is never automatically safe. But it has no reachable call
site here: nothing anywhere is keyed by EntityStore / InstanceCollection / ChangeItem
(zero hits in key position across src/ and test/), and the reactive hot path uses the
parameterless DistinctUntilChanged(), which resolves through EqualityComparer<T>.Default.Equals
and never calls GetHashCode. Equals is deliberately unchanged by this PR, so the per-emission
cost of that pipeline is exactly what it was.

Note also which direction this fix moves the cost: the production stacks were entered by hashing a
stream — SynchronizationStream's synthesized hash walks its private ChangeItem<TStream> current, so cycle 1 was the entry point into cycle 2. With streams now hashing by reference, those
container hashes run less often than before, not more.

No allow-list entry was added: the gate's 0 stale allow entr(ies) ratchet is working as designed
and must not be papered over.

Two independent hash cycles were killing portal pods with an uncatchable StackOverflowException. Both are semantic defects — hashing something that was never a value — not missing depth guards, so both are fixed by giving each type the equality semantics it actually has. Cycle 1 (#2163/#2164/#2172/#2173/#2174/#2175) — record-synthesized structural hashing over a back-reference. SynchronizationStream<T> is a record, so the compiler walked every instance field including Configuration; StreamConfiguration<T>'s primary-ctor property Stream points straight back at the owning stream (new StreamConfiguration<T>(this)). Hashing a stream therefore ran StreamConfiguration.GetHashCode -> SynchronizationStream.GetHashCode -> ObjectEqualityComparer.GetHashCode -> ... with no base case, exactly the captured production stack. A stream is a live identity object — it owns a ReplaySubject, mutable current state, a hosted hub and disposal state — so it now compares and hashes by reference, and a configuration hashes its own settings while treating its owner as a reference. Both stay records; the with-based builder API is untouched. Cycle 2 (#2170/#2171) — hand-written structural hashing over an arbitrary object graph. InstanceCollection hashed its VALUES (arbitrary user objects), EntityStore aggregated its collections, and ChangeItem<T>'s synthesized hash walked its Value, so a stored instance reaching back into the store closed the loop InstanceCollection -> EntityStore -> ChangeItem -> InstanceCollection. These now hash from keys, names and counts — bounded, cycle-free, and no longer O(the entire graph) on a routing hot path. Keys are safe by construction: the dictionary already hashed every one of them to store it. Also fixes a latent crash found alongside: EntityStore.GetHashCode called Aggregate with no seed, so an EMPTY store threw InvalidOperationException ("Sequence contains no elements"). Every value-based Equals is deliberately left intact — the SetCurrent dedup that suppresses re-emission of an identical re-synced store depends on it. The weaker hashes stay consistent with those Equals implementations (equal objects still hash equal), which is all the contract requires. HashRecursionTest builds the real cyclic graphs and pins the semantics. Verified red before the fix: the two store tests fail cleanly (the cycle-closing probe reports re-entry; the empty store throws), and the stream test crashes the test host with SIGABRT (exit 134) — the StackOverflow itself. All 8 pass after. Closes #2163, #2164, #2170, #2171, #2172, #2173, #2174, #2175 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes two production crash loops caused by unbounded GetHashCode() recursion in the data synchronization layer, by redefining equality/hash semantics to match the true identity/value boundaries (streams by reference identity; stores/collections by bounded key/size hashing).

Changes:

Make SynchronizationStream<TStream> hash/compare by reference identity, and make StreamConfiguration<TStream> compare settings while treating its owning stream back-pointer by reference (breaking the stream↔configuration cycle).
Replace InstanceCollection / EntityStore / ChangeItem<T> hashing with bounded, cycle-free hashes (keys, names, counts, and identity fields) and fix the empty-store .Aggregate() crash.
Add regression tests that deterministically fail pre-fix (without risking StackOverflowException killing the test host) plus a What’s New entry.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/MeshWeaver.Data.Test/HashRecursionTest.cs	Adds regression coverage for both recursion cycles and the empty-store hashing crash.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-25-hash-recursion-pod-crash.md	Documents the user-visible fix (pod no longer crashes) in What’s New.
src/MeshWeaver.Data/Workspace.cs	Updates the stream reference comparer comment to reflect the new semantics and why it remains relevant for interface keys.
src/MeshWeaver.Data/Serialization/SynchronizationStream.cs	Implements reference-identity equality/hash for `SynchronizationStream<T>` and bounded equality/hash for `StreamConfiguration<T>`.
src/MeshWeaver.Data/ChangeItem.cs	Overrides `ChangeItem<T>.GetHashCode()` to avoid hashing the full `Value` graph.
src/MeshWeaver.Data.Contract/InstanceCollection.cs	Reworks `InstanceCollection.GetHashCode()` to hash keys/count only (bounded, cycle-free).
src/MeshWeaver.Data.Contract/EntityStore.cs	Reworks `EntityStore.GetHashCode()` to hash collection names + sizes and avoids empty-sequence `Aggregate()` failure.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-08-25T18:08:47Z