Repository navigation
fix: storm breaker key carries payload identity, so a wide fan-out is not dropped as a loop - #1266
Conversation
… not dropped as a loop
The hub-ingestion MessageStormBreaker keyed its per-second rate counters on
(sender, target, message-type) alone. Its own justifying comment claimed real traffic is
"DIVERSE tuples" — that is false wherever the mesh funnels traffic through a dispatcher:
ONE sender talks to ONE target with ONE message type about MANY DIFFERENT THINGS. Every
sync stream an owner hub holds open to the shared mesh-node cache posts
DataChangedEvent/RawJson over that one tuple; a bulk importer sends one
CreateOrUpdateNodeRequest per source node from one hub to the mesh hub.
Keyed on the routing tuple alone, a wide LEGITIMATE fan-out is arithmetically identical to
one thing looping. So the guard crossed its bar on ordinary bulk work and ShouldDrop
discarded real writes at ingestion — imported content and progress-log lines silently
missing. That is data loss, not mitigation.
The key now carries the payload's own identity, resolved without ever parsing a payload on
the ingestion path:
* typed message -> IDiagnosticKeyed.DiagnosticKey (an already-computed stream id / node
path). Added to StreamMessage (StreamId — covers DataChangedEvent,
PatchDataChangeRequest, UnsubscribeRequest, StreamErrorEvent) and to the node-lifecycle
verbs Create / CreateOrUpdate / Delete / Copy / Move (the node path).
* after a hub hop the payload type is erased to RawJson, so MessageDelivery.Package
stamps the key onto the ENVELOPE right before it serializes — one interface check next
to a full JSON serialize, and delivery properties already survive both the router hop
and the wire.
* no identity exposed -> the key degrades to exactly the old tuple. The fallback is the
old, stricter behaviour: fail closed, never "allow".
Threshold, window and cooldown are untouched; a genuine single-thing loop still trips
exactly as before, and the Error now names WHICH stream/path was looping instead of only
which two hubs were talking.
The payload component makes the key space unbounded in principle (the old
"evict when the key is next touched" never fires for a key seen once), so crossing a soft
cap of 10k live counters arms one inline sweep per window that drops counters idle beyond
window+cooldown. The live set then tracks recent traffic rather than everything the hub
ever saw — no timer, no background thread, no static state, and no effect on detection
since an idle key is by definition not storming.
Tests (deterministic, injected clock): a wide fan-out over one tuple never trips; the same
volume concentrated on one thing still trips and names it; the identity survives Package's
type erasure; an unidentifiable payload keeps the old tuple key and still trips; tracked
keys stay bounded by recent traffic. Proven red before the fix — with the payload
component removed, ShouldDrop returns true for a write to a DISTINCT path.
Closes #1200
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-loss defect in MessageStormBreaker where legitimate high-volume fan-out traffic (many distinct payloads sharing the same sender/target/type tuple) was misclassified as a looping storm and dropped at ingestion. The breaker’s key now includes a payload identity component (when available) and preserves that identity across cross-hub packaging to RawJson, with tests and docs updated accordingly.
Changes:
- Extend the storm-breaker discriminator from
(sender, target, type)to(sender, target, type, payload-identity)usingIDiagnosticKeyed.DiagnosticKeyand a packaged envelope property forRawJson. - Add bounded-key-space logic (soft cap + inline stale sweep) to prevent unbounded counter accumulation when payload identity introduces many distinct keys.
- Add deterministic tests covering wide fan-out vs. true loops, packaging type-erasure behavior, fallback behavior, and key tracking bounds; update “What’s New” and architecture docs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/MessageStormBreakerTest.cs | Adds deterministic coverage for wide fan-out discrimination, loop detection, packaging preservation, fallback behavior, and stale-sweep bounding. |
| src/MeshWeaver.Messaging.Hub/MessageStormBreaker.cs | Adds payload identity into the storm key, introduces tracked-key bounding via inline stale sweeps, and improves trip diagnostics to name the offending payload identity. |
| src/MeshWeaver.Messaging.Hub/MessageDelivery.cs | Stamps IDiagnosticKeyed identity into delivery properties during Package() so payload identity survives type erasure to RawJson. |
| src/MeshWeaver.Messaging.Contract/IDiagnosticKeyed.cs | Documents the interface as load-bearing for storm-breaker discrimination and defines the delivery property name used across packaging. |
| src/MeshWeaver.Mesh.Contract/CreateNodeRequest.cs | Implements IDiagnosticKeyed on node lifecycle verbs so bulk node operations discriminate per-path rather than folding into one storm key. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-12-bulk-imports-no-longer-lose-content-to-the-storm-guard.md | Adds a user-visible “Fix” entry describing the resolved import/sync data-loss behavior and improved diagnostics. |
| src/MeshWeaver.Documentation/Data/Architecture/ActionBlockWedgePrevention.md | Updates architecture guidance to clarify that “per-key” includes payload identity and explains why. |
| src/MeshWeaver.Data.Contract/Messages.cs | Implements IDiagnosticKeyed on StreamMessage so stream traffic keys by StreamId for storm-breaker discrimination. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…le keys too Two real defects in the previous commit, both found by review: 1. GetOrAddCounter could return a counter that was never published. TryGetValue miss → TryAdd loses the race → the winner's entry is evicted (window roll / sweep) before the re-read → the fallback handed back the locally-built instance. That delivery would then be counted on an orphan nobody else can see, silently resetting a storming key's count and cooldown. Replaced with a retry loop that only ever returns a counter it has observed in, or successfully published to, the dictionary. 2. SweepStaleKeys skipped Tripped counters, so a key that stormed once and was never seen again lived for the hub's lifetime — defeating the very bound the sweep documents. Staleness is now the only criterion. It is also the correct one: "stale" means no message for a full window+cooldown, which an actively storming key can never be (every message restamps its window) and which is already past the cooldown, so releasing the counter is behaviourally identical to the self-heal its next message would perform. Test pins it, including that the formerly-storming key flows again afterwards. The opportunistic in-ShouldDrop eviction keeps its !Tripped guard — that path fires on a zero-count window roll for a key still being touched, which may legitimately be mid-cooldown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…th-discrimination
|
Both review findings were real defects in this change and are fixed in 4097afd:
The opportunistic in-
|
CI is RED on shard 2 — analysis: not caused by this diffWhat actually failed. That is a crash inside Roslyn's metadata writer while emitting the NodeType assembly — StaleBarrierType, CodeEditType, EmptyRaceType, ButtonType, DirtyType and BrokenType all die identically, i.e. a process-wide condition, not per-type source. Why this diff cannot be the cause — provable, not asserted. The job log contains zero The only other behavioural delta in the diff is Local evidence on this exact commit (4097afd):
Controls checked (same signature searched, not found): main runs 31580689228 and 31577940567 (shard 2 red for unrelated reasons — "Area not found", observable timeouts),
Not merging — a coordinator runs the merge train, and the gate is green CI. |
…th-discrimination
Shard 2 attribution — checked reachability both ways, then merged mainWhat failed: i.e. a crash inside Roslyn's metadata writer, process-wide rather than per-type. I did not assume "known flake". This diff touches
None of those paths reaches Controls (same Roslyn signature searched, absent in all): main runs 31580689228 and 31577940567 — shard 2 red for different reasons — plus Action taken: merged current Local on the merged tree: |
Closes #1200
The defect
MessageStormBreakerkeys its per-second rate counters on(Sender, Target, TypeName)— nocomponent identifying what the message is about. Its own justifying comment claims real traffic
is "DIVERSE tuples". That is false wherever the mesh funnels traffic through a dispatcher: one
sender talks to one target with one message type about MANY DIFFERENT THINGS.
DataChangedEvent(→RawJsononce the router packages it) over that single tuple.CreateOrUpdateNodeRequestper source node from one import hub to theone mesh hub.
Keyed on the routing tuple alone, a wide legitimate fan-out is arithmetically identical to one
thing looping. The guard therefore crosses its bar on ordinary bulk work and
ShouldDropdiscards real writes at ingestion — imported content and progress-log lines silently missing.
That is data loss, not mitigation. The issue text credits the guard with "mitigating" the
storm and rates the impact "low"; it is backwards. The trip is the guard eating legitimate traffic,
and the reason nobody could tell is the same missing component: with no payload identity in the
key or the log line, "N things fanning out" and "one thing looping" produce an identical message.
The issue's stated cause is refuted. It blames router-identity miswiring (#1140). Its own log
disproves that:
sender=UWDeepfield/_Activity/import-…-f4dc71b1is the import attempt node's ownper-node hub (the fresh-per-run node
StaticRepoImporterlogs progress to), not the router. Therouter appears only as the hub that packages and forwards the frames — which is exactly why the
cache hub sees them as
type=RawJson.The fix
The key gains a fourth component: the payload's own identity, resolved without ever parsing a
payload on the ingestion path (the breaker sees every inbound delivery, before the turn loop — a
per-message parse would be its own performance defect).
IDiagnosticKeyed.DiagnosticKey— an already-computed propertyRawJson, after a hub hop erased the typeMessageDelivery.PackageRawJsonnull→ key degrades to exactly the old tupleIDiagnosticKeyedalready existed for the pending-callback diagnostic, for precisely this"N distinct things vs one thing repeated" ambiguity — this makes it load-bearing. It is now
implemented by
StreamMessage(StreamId, coveringDataChangedEvent,PatchDataChangeRequest,UnsubscribeRequest,StreamErrorEvent) and by the node-lifecycle verbsCreate/CreateOrUpdate/Delete/Copy/Move(the node path).Package()is the one seam where the type is erased for a cross-hub hop, and it is already doing afull
JsonSerializer.Serialize— stamping the key there is free by comparison, and deliveryproperties already survive both the in-process router hop and the gRPC/SignalR wire (the same
mechanism that carries
PostOptions.RequestId).No band-aid: threshold (2000), window (1 s) and cooldown (2 s) are untouched, no exemption
list, no widened bound. A genuine single-thing loop still trips exactly as before — and the
Errornow names which stream/path was looping instead of only which two hubs were talking.
Bounded key space
The payload component makes the key space unbounded in principle: the existing eviction only fires
when a key is touched again after an idle window, so a key seen once would live forever. Crossing
a soft cap of 10 000 live counters arms one inline sweep per window that drops counters idle
beyond window+cooldown. The live set then tracks recent traffic instead of everything the hub
ever saw — no timer, no background thread, no static state, and no effect on detection (a key idle
for seconds is by definition not storming).
Tests
Deterministic, injected logical clock — no sleeps, no wall-clock races
(
MessageStormBreakerTest):WideFanOut_OneTuple_ManyDistinctPayloadKeys_NeverTrips— the bug. 40× the threshold throughONE tuple, each message about a different path → must not trip.
RepeatedSamePayloadKey_StillTrips_AndNamesTheThing— the guard is intact; the trip names thething; and a fan-out running alongside the loop keeps flowing (the drop is scoped to the one
storming key, not to everything sharing its sender/target/type).
PackagedDelivery_KeepsPayloadIdentity_AcrossTheTypeErasure— the same fan-out survivesPackage(); a packaged loop still trips, reported astype=RawJsonwith the payload named.NoPayloadIdentity_FallsBackToTheRoutingTuple_AndStillTrips— the fallback is fail-closed.TrackedKeys_AreBounded_ByRecentTrafficNotAllTimeTraffic— 500 one-shot keys are swept.Red-before proof (payload component neutered, everything else identical):
Local verification (CI parity)
dotnet build MeshWeaver.slnx -c Release -p:CIRun=true -warnaserror→ 0 Errors, 0 Warnings(re-run after merging current
main).MeshWeaver.Messaging.Hub.Test→ 191/191MeshWeaver.Data.Test→ 311/311MeshWeaver.Hosting.Monolith.Test→ 582 passed / 3 pre-existing skipsMeshWeaver.Documentation.Test→ 20/20What's New entry included (
Category: Fix— a user notices imported content going missing).🤖 Generated with Claude Code