Give the Azure Service Bus infrastructure factory an honest receiver lifetime (#376) - #503
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf30837dc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…in ADR-0022 The scoped receiver was itself disposed by the factory's scope; only the injected graph and the not-yet-built inner receiver were latent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U7t1aWmGQZNjZi8FQURV6G
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc40d86067
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bccc28e44a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…instance Sole-disposer ownership is true of the ServiceBusReceiver INSTANCE and stops there. The container still constructs, and so still disposes, the shared ServiceBusClient the receiver receives THROUGH, and that client is resolved lazily during the first receiver construction — after the hosted service — so LIFO disposal reaches it FIRST on the same provider-disposed-without-StopAsync path the eliminated class was argued over. Record it as an accepted residual rather than widen the fix: the path is traced end to end and cannot reproduce the eliminated class, because no link can be rebuilt behind a latched _disposedValue — the rebuilt adapter opens nothing in its constructor and the SDK's own not-disposed assert throws out of CreateReceiver. What is left is one Error-level line during a teardown that had already skipped its stop. The three obvious remediations are rejected on the merits: the client has no single pump-bounding owner (N receivers, one client, shared by contract for cross-entity transactions), and a disposed-client guard buys nothing the SDK assert does not already raise. Scope the Closed-by-Construction claim to the instance it is true of, so the prose stops one step short of the code instead of one step past it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U7t1aWmGQZNjZi8FQURV6G
1 participant
Closes #376. #376 was the last open child of epic #307, so this closes that epic too.
The defect
The singleton
IMessagingInfrastructureregistration inChatterAzureServiceBusExtensionsbuilt itsMessagingInfrastructureFactoryfrom two delegates that each did:The
usingdisposed the scope before the resolved instance was handed to the caller.ServiceBusReceiverwas registeredAddScoped, so the scope owned that instance and disposed it during teardown — every caller received an already-disposed receiver. It was benign only by accident of timing:_innerReceiverhad not been lazily built yet, and nothing checked the disposed flag.The fix
Every dependency of
ServiceBusReceiverandServiceBusMessageSenderis a singleton — verified against the tree, including the options types registered throughAddBuiltOptions. With nothing scoped in the graph, the honest shape is to open no scope at all:IServiceProvider. TheIServiceScopeFactorylookup is gone.AddScopedtoAddTransient. Singleton would be wrong:InitializeAsyncwrites instance state and one receiver is created per receiver entity, so a shared instance would cross-wire entities.This is not a new rule. The core module already landed it under #314 (epic #297) and states it as an invariant in
ChatterMessageBrokerExtensions: a registration factory must never open a DI scope whose resolved graph escapes the factory delegate. ADR-0022 records the Azure Service Bus specialization of that rule so the remaining sibling sites inherit one shape rather than growing a third.Two corrections to the issue
ServiceBusMessageSenderimplements onlyIMessagingInfrastructureDispatcher, which is notIDisposable, so MS DI never tracked it and the scope never disposed it. Its only defect was per-access scope churn. The issue's "same forServiceBusMessageSender" is wrong in kind.SqlServiceBrokeris not the same case. It carries a near-identical shape, butISqlConnectionSourceis registeredScopedthere, so its graph genuinely holds a scoped member. The transient-plus-root answer must not be copied onto it — the component must own the scope instead. ADR-0022 says so explicitly, along with the RabbitMQ sender site tracked as Per-message DI scope creation on the publish path, and sender resolved from an already-disposed scope #371.Teardown observation
Fixing the lifetime made the receiver's synchronous
Disposepath worth looking at. It firedCloseAsync()without awaiting or observing it, so a failed close was an unobserved faulted task.The first attempt handled the two failure shapes anyone thinks of. Review then found two more — a
CloseAsync()returningnullthrewNullReferenceExceptionout ofDispose, and a cancelled task never ran theOnlyOnFaultedcontinuation, so an aborted close read as a clean teardown. Enumerating outcomes is the defect, not the specific outcomes missed.So the close helper no longer names outcomes at all. It is one guarded region spanning both obtaining the task and attaching the continuation, plus one unfiltered continuation that reports unless the terminal state is exactly
RanToCompletion— a positive allowlist of one success state rather than a list of failures. A newTaskStatusneeds no new branch. A single non-throwing reporter serves both regions, so a failing logger can no longer escapeDispose.The guarantee is over which outcomes get reported, not over the report line landing:
ExecuteSynchronouslyis a hint the runtime may decline, and a queued report can be lost at process exit. ADR-0022 records that as an accepted residual with its rejected alternatives rather than overstating the guarantee.Validation
Per-project, never solution-wide.
Chatter.MessageBrokers.AzureServiceBus.TestsChatter.MessageBrokers.TestsBaseline at
8b58127was 445 passed on the Azure Service Bus suite; the 16 added facts are the new tests. The 2 skips are the Docker-gated integration tests that skip at discovery, unchanged. No public surface moved — every touched type isinternal.Versioning
Chatter.MessageBrokers.AzureServiceBus2.5.1 → 2.5.2 (patch). The only consumer-visible delta is a new Warning log line when a teardown close does not succeed.