GH-4510: do not treat a sticky-bound listen-only endpoint as a local send target - #4553
Merged
Merged
Conversation
…send target Reported by @BaharAtNode against 6.39.1, with the root cause already traced in the issue. PrepopulateRoutingCache (new in 6.0) walks every discovered message type during StartAsync. LocalTransport.DiscoverSenders yielded every endpoint a handler chain was sticky-bound to as a local send candidate, so building the route called CreateSender on a receive-only endpoint -- an Azure Service Bus subscription, in the report -- which threw NotSupportedException and took the whole host down. Every environment, every start. opts.ListenToAzureServiceBusSubscription("my-subscription") .FromTopic("my-topic") .RequireSessions() .ProcessInline() .AddStickyHandler(typeof(MyHandler)); is a documented pattern for fanning one topic out to several session-scoped, exclusively bound handlers, so this was not a narrow edge case. It is a 6.x regression: on 5.x routes were built lazily on first send, so a message type that was only ever received never hit this path at all. A sticky binding means "deliver this message type to this listener". It was never a claim that anything can send to the endpoint, and conflating the two is the bug. Endpoint gains `protected internal virtual bool supportsSending => true` -- default open, following the supportsNativeAck / supportsRedelivery pattern -- overridden to false on the two listen-only endpoints that previously answered CreateSender by throwing: AzureServiceBusSubscription (publish to its topic) and KafkaTopicGroup (publish to the topic). LocalTransport.DiscoverSenders skips them. Structural rather than catching the throw, as the issue's second suggestion allows, because it also keeps such an endpoint out of routing tables and diagnostics where it never belonged -- and a new listen-only endpoint on any transport gets the right behavior by overriding one property instead of relying on a catch. The regression test earns its keep: with the guard removed it fails with the reporter's exact NotSupportedException. Note it needs TWO handlers for the message type, because sticky assignment only populates chain.ByEndpoint when `grouping.Count() > 1` -- which is also precisely the fan-out shape the report describes. A first version with one handler passed both with and without the fix and proved nothing. CoreTests: 3032 passed, 2 skipped, 0 failed. Wolverine.Kafka.Tests: 309/309. Azure Service Bus suite failures on this machine are emulator saturation -- the same classes fail identically on an origin/main build. Closes #4510 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj
jeremydmiller
added a commit
that referenced
this pull request
Sep 23, 2026
`unknown_tenant_problem_details_4516.without_the_opt_in_the_unknown_tenant_still_escapes` went red on CI the moment #4551 merged, and it takes the whole `test` job with it -- so every open PR is red regardless of its contents (#4553, #4557, #4558 at the time of writing, two of them from contributors). The test was self contradictory. It asked Alba to assert a 404 inside a scenario it expected to THROW: await Should.ThrowAsync<UnknownTenantIdException>(async () => await host.Scenario(x => { x.Get.Url("/gh4516/tenanted?tenantId=ghost"); x.StatusCodeShouldBe(404); // <- asserted on a request expected to blow up })); Locally the UnknownTenantIdException propagated before Alba evaluated its assertions, so the expected exception won and the test passed. On CI the host turned the exception into a 500 first, so Alba's own 404 assertion fired and raised ScenarioAssertionException instead of UnknownTenantIdException -- a different type, so Should.ThrowAsync failed. The control's actual purpose is narrower than what it asserted: it exists to show that MapUnknownTenantToNotFound() is what produces the 404, not something else in the pipeline. It is not a claim about HOW an unmapped failure surfaces, and that is exactly the part that varies by environment. It now uses IgnoreStatusCode() and asserts only that the response is not a mapped 404, tolerating the other surfacing -- the exception escaping the scenario -- and nothing else. Claude-Session: https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4510. Left unmerged for your review — this one was reported by @BaharAtNode rather than raised in our own sweep.
Credit where it's due: the issue traced the root cause exactly, down to the two lines involved. This implements the first of its two suggested directions.
The bug
PrepopulateRoutingCache(new in 6.0) walks every discovered message type duringStartAsync.LocalTransport.DiscoverSendersyielded every endpoint a handler chain was sticky-bound to as a local send candidate, so building the route calledCreateSenderon a receive-only endpoint — an Azure Service Bus subscription — which threwNotSupportedExceptionand took the whole host down. Every environment, every start.This is a 6.x regression: on 5.x routes were built lazily on first send, so a message type that was only ever received never hit this path.
A sticky binding means "deliver this message type to this listener". It was never a claim that anything can send to the endpoint, and conflating the two is the bug.
The fix
Endpointgainsprotected internal virtual bool supportsSending => true— default-open, following the existingsupportsNativeAck/supportsRedeliverypattern — overridden tofalseon the two listen-only endpoints that previously answeredCreateSenderby throwing:AzureServiceBusSubscription(you publish to its topic)KafkaTopicGroup(you publish to the topic)LocalTransport.DiscoverSendersskips them.I went structural rather than catching the throw, which the issue's second suggestion also allows, because it keeps such an endpoint out of routing tables and diagnostics where it never belonged — and a new listen-only endpoint on any transport gets the right behaviour by overriding one property instead of relying on someone remembering a catch.
The regression test is a real one
With the guard removed it fails with the reporter's exact
NotSupportedException. Worth knowing for anyone touching this area: it needs two handlers for the message type, because sticky assignment only populateschain.ByEndpointwhengrouping.Count() > 1(HandlerChain.cs:130) — which is also precisely the fan-out shape the report describes. My first version used one handler, passed both with and without the fix, and proved nothing.The test uses a local stub endpoint rather than a real broker, so the regression is pinned in
CoreTestswithout needing Azure or Kafka.Verification
The Azure Service Bus suite fails ~13 tests on my machine, but the same classes fail identically on an
origin/mainbuild — the emulator is saturated (Service is warming up, tracked-session timeouts). Not from this change. Worth a clean-machine CI run to confirm.🤖 Generated with Claude Code
https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj