An unsubscribe's sender is correlation state, not router traffic (#4489) - #4498
Conversation
…traffic #1140's residue, split out as #4489. PR #4487 hopped every router-as-an-end site that had a seam to hop onto; this one deliberately does not, because the sender is not incidental. JsonSynchronizationStream.CreateExternalClient binds `var hub = workspace.Hub` once and posts BOTH halves of the pair on it — the SubscribeRequest at postSubscribeRequest, the release UnsubscribeRequest in its disposable. The owner's per-subscriber stream is keyed on the subscriber that opened it, so hopping only the unsubscribe leaves a subscription opened by one hub and released by another; hopping both changes SubscribeRequest.Identity, which is what the owner's access check reads, and whose documented failure mode is a denied-subscribe flood. So a mesh/{id} sender there is CORRECT, and the detector should stop demanding a change nobody may make — the same argument the rule already makes for HeartBeatEvent and the routing NACK. Carried by a marker on the message (ICorrelatedBySender) rather than an allow-file line, so it travels with the contract and a rename cannot detach it. A type check, not a name match, per the HeartBeatEvent exclusion's own reasoning; the marker lives in Messaging.Contract because Data.Contract references it and not the reverse. It is exactly as narrow as the site — `new UnsubscribeRequest(` appears once in all of src/. Controls fired: removing the branch reds AMessageCorrelatedByItsSender 1/1 and AnUnsubscribeIsCorrelatedByItsSender 1 of 2 — the release test reds while the SubscribeRequest control keeps passing, so the pair discriminates rather than moving together. Restored: 15/15 and 2/2, four projects Release -warnaserror with 0 warnings 0 errors. Fixes #4489 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Receiver-side envelope handling still reports correlated unsubscribe traffic, and related scope, documentation, and compatibility tests need correction.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds a correlation marker so UnsubscribeRequest is excluded from router-traffic diagnostics.
Changes:
- Adds and applies
ICorrelatedBySender. - Extends router-traffic and unsubscribe tests.
- Documents the decision and adds a release note.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/RouterTrafficRuleTest.cs |
Tests marker behavior and control cases. |
test/MeshWeaver.Data.Test/AnUnsubscribeIsCorrelatedByItsSenderTest.cs |
Verifies unsubscribe and subscribe distinctions. |
src/MeshWeaver.Messaging.Contract/RouterTrafficRule.cs |
Adds the marker-based exclusion. |
src/MeshWeaver.Messaging.Contract/ICorrelatedBySender.cs |
Defines the correlation marker. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-16-a-diagnostic-stops-reporting-something-nobody-can-change.md |
Adds the release note. |
src/MeshWeaver.Documentation/Data/Architecture/RouterTrafficDetection.md |
Records the architectural decision. |
src/MeshWeaver.Data.Contract/Messages.cs |
Applies the marker to UnsubscribeRequest. |
Review details
Suppressed comments (3)
src/MeshWeaver.Documentation/Data/Architecture/RouterTrafficDetection.md:87
- The table above explicitly says the receiver sees
RawJsonafter a silo hop, so this new claim that the marker is excluded at both detector sites is not true for the receiver path: that site evaluates the packaged payload before it is unpacked. Please document the actual boundary only after carrying the exclusion through the envelope, or add that envelope-level implementation in this change.
**A delivery the receiver correlates BY ITS SENDER.** A message implementing
`ICorrelatedBySender` is excluded at both sites. The claim it carries is about the RECEIVER: some
earlier delivery already told that receiver to remember this sender, and this one is only meaningful
against that memory — so re-posting it from an off-router hub, the one change that would silence the
report, is precisely what breaks the pairing.
src/MeshWeaver.Messaging.Contract/RouterTrafficRule.cs:54
- The marker is checked before the router-end roles are calculated, so it suppresses a
targetviolation as well as the sender-correlation case. For example, an implementing message from a non-router hub tomesh/...is still work addressed to the router and should remain reportable (the ordinary control atRouterTrafficRuleTest.RouterAsAnEnd_IsReportedpins this role). Please narrow the exclusion to the sender contribution, or use a marker whose contract explicitly exempts the entire delivery; the current all-positions test makes this blind spot permanent.
if (message is ICorrelatedBySender)
return null;
test/MeshWeaver.Messaging.Hub.Test/RouterTrafficRuleTest.cs:74
- Although the comment says this covers the shipped three-argument overload, the call passes
isResponseand therefore exercises the four-argument overload again. The legacy overload is not covered by the new exclusion test; call the three-argument form here so the claimed binary-compatibility path is actually pinned.
// …and on the 3-argument form too, which is the signature shipped assemblies still bind to.
Assert.Null(RouterTrafficRule.RoleOf("portal", Mesh, new ACorrelatedRelease(), isResponse: false));
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (message is ICorrelatedBySender) | ||
| return null; |
| There is exactly one implementer, and it is the residue #4487 deliberately left behind (#4489, split | ||
| from #1140): the `UnsubscribeRequest` posted by `JsonSynchronizationStream.CreateExternalClient`'s |
|
ℹ️ Merge-queue steward: no action — removed from the queue with reason |
Copilot found a false claim, and I had the evidence to know better: I wrote "excluded at both sites" while my own live measurement on #4489, posted the same day, is the pair that disproves it — ROUTER_TRAFFIC ORIGIN: DisposeRequest was POSTED with the mesh hub as sender ROUTER_TRAFFIC: RawJson has the mesh hub as sender ReportRouterTraffic runs at MessageHub.DeliverMessage:2007, BEFORE RouteMessageAsync unpacks at :2009, so a cross-hub delivery is still RawJson when the receiver-side detector reads it and no message-typed exclusion can match. The tests passed because they call RoleOf directly with a typed message and never through the packed path. Narrowed the claim rather than carrying the marker through the delivery envelope: putting a detector concern into the wire format for one message is the worse trade. The ORIGIN line is the one #4489's evidence names and the only one carrying a call site anyone can act on; a receiver-side RawJson line names no type and no call site and prints for every cross-hub delivery with the router as an end — pre-existing, not opened here. New test TheExclusionCannotReachAPackedDelivery asserts a packed RawJson carrying an unsubscribe IS still reported, so the boundary is pinned and the next reader cannot assume what I assumed. Also fixed the second finding: the page still carried a #4489 residue block saying the residue "stays open and is deliberately not fixed here", giving opposite instructions to the new section. It now records the decision and points at the scope note. Verified: 3/3 and 15/15, four projects Release -warnaserror, 0 warnings 0 errors. Dequeued from the merge queue to make these rather than land first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both findings are correct. I dequeued this PR from the merge queue to fix them rather than let it 1. The exclusion does not reach the receiver site — confirmed, and I had already measured itVerified in the code rather than taken on trust: // MessageHub.DeliverMessage
ReportRouterTraffic(delivery); // :2007
var ret = delivery.ChangeState(MessageDeliveryState.Submitted);
var result = messageService.RouteMessageAsync(ret, default); // :2009 ← unpacking happens hereAnd my own live reading on What I changed, and what I deliberately did not. I narrowed the claim instead of carrying the So: the doc's "excluded at both sites" is gone, replaced by a scope section that quotes the call I did not add a cross-hub integration test: the unit assertion above pins the same fact at the 2. The stale #4489 residue block — fixedYou are right that one page gave opposite instructions. The older block still read "One named |
Fixes #4489. Takes the second of the two paths that issue lists under "What done looks like" —
a reasoned decision that this delivery's sender must stay the stream's own hub, recorded in
Router Traffic Detection
and carried as an exclusion in
RouterTrafficRule, the shared predicate both detector sitesevaluate.
Why the second path and not the first
The first path — move the unsubscribe and its subscribe to the same off-router hub — is not
available at an acceptable price, and the code says so at the site:
JsonSynchronizationStream.CreateExternalClientbindsvar hub = workspace.Hubonce and postsboth halves on it: the subscribe at
postSubscribeRequest, the release in its disposable. Theowner's per-subscriber stream is keyed on the subscriber that opened it, so hopping only the
unsubscribe leaves the owner holding a subscription opened by one hub and released by another —
exactly what ROUTER_TRAFFIC residue: JsonSynchronizationStream releases its subscription as the router, and hopping it is a correlation decision (split from #1140) #4489 warned about.
SubscribeRequest.Identity. That file callsMintSubscribeRequest"THEONE PLACE a C# subscriber mints a SubscribeRequest", and documents the failure mode of getting its
identity wrong: a wiped
AccessContextfails closed → the owner denies the subscribe → theconsumer re-opens → a flood of denied
SubscribeRequests.So the sender here is correlation state, not an incidental origin, and a
mesh/{id}sender onthis release is correct. Reporting it asks for a change nobody may make, which is the shape that
trains people to mute the channel — the same argument the rule already makes for
HeartBeatEventand for the routing NACK.
The shape of the exclusion
A marker interface in
MeshWeaver.Messaging.Contract, implemented byUnsubscribeRequest:HeartBeatEventexclusion's own reasoning that"a rename must not silently turn this exclusion off". A direct
message is UnsubscribeRequestis impossible: that type lives in
MeshWeaver.Data.Contract, which referencesMeshWeaver.Messaging.Contractand not the reverse.contract and is visible where the decision applies.
new UnsubscribeRequest(appears once in all ofsrc/,at the site in question.
The interface's doc says what implementing it claims, because the risk of a marker is that someone
uses it to quiet an inconvenient report: it is a statement about the RECEIVER — that an earlier
delivery already told it to remember this sender — and a message whose sender the receiver does not
correlate has no business carrying it.
Verification
Core, Release with
-warnaserror,0 Warning(s) 0 Error(s)on all four touched projects;RouterTrafficRuleTest15/15 and the newAnUnsubscribeIsCorrelatedByItsSenderTest2/2.The controls fired. Removing the
ICorrelatedBySenderbranch from the rule and rebuilding:AMessageCorrelatedByItsSender_IsNeverReportedAnUnsubscribeIsCorrelatedByItsSenderTestThe 1-of-2 is the discriminating part:
TheReleaseCarriesTheClaimreds, whileTheSubscribeDoesNotCarryIt_AndIsStillReportedkeeps passing — so the pair distinguishes theexclusion from the ordinary behaviour rather than moving together. Restoring the branch returns both
to green.
Two tests keep the exclusion from widening:
AnOrdinaryMessageInTheSamePositions_IsStillReportedpins that a plain message is still reported as
target/sender/sender AND target, and thelink test pins that
SubscribeRequestdeliberately does not carry the marker — silencing bothhalves would remove the detector's view of subscription traffic entirely.
The live measurement behind it
Posted on #4489 earlier today: over a 700-minute window covering both
memexpods' startup, theorigin detector reported
DisposeRequestandRawJsonand notUnsubscribeRequest. Thatsupports the site being theoretical as it stands, but it is deliberately not the argument for
this change — the detector dedups once per role+type per process lifetime, so a clean window proves
"not since these pods started", never "never occurs". The argument above stands on the code.
Pairs-with: none — no public type or member is removed.
ICorrelatedBySenderis a new markerinterface with no members (so nothing to declare under the interface-additions gate), and
UnsubscribeRequestgains an interface, which is additive.Mirror-sync: none — no localization key added or re-worded.
🤖 Generated with Claude Code