fix: release per-message transformers and mappers to their factories

[iancooper]

Summary

Two related defects in how Brighter returns per-message objects to their factories.

Part A — transient transformer leak (Fixes #4252)

• ServiceProviderLifetimeScope.GetTransient<T> now creates a fresh IServiceScope per call, tracked by instance identity, and disposes it in Release — closing the unbounded-growth and double-dispose defect for transient transforms.
• GetOrCreateScoped is decoupled from GetTransient so scoped resolutions continue to share the existing _scope field, unaffected by this change.

Part B — message mappers had no release path at all (raised in the code review of this PR)

• IAmAMessageMapperFactory and IAmAMessageMapperFactoryAsync gain Release, so a mapper can be returned to its factory exactly as a transformer is.
• The transform pipeline now owns its mapper and releases it on disposal, and the five production call sites dispose the pipeline deterministically instead of relying on ~TransformPipeline().

⚠️ Breaking change

Release is added to four public interfaces:

• IAmAMessageMapperFactory — void Release(IAmAMessageMapper mapper)
• IAmAMessageMapperFactoryAsync — void Release(IAmAMessageMapperAsync mapper)
• IAmAMessageMapperRegistry — void Release(IAmAMessageMapper mapper)
• IAmAMessageMapperRegistryAsync — void ReleaseAsync(IAmAMessageMapperAsync mapper)

Brighter targets netstandard2.0, which has no runtime support for default interface members, so these cannot ship with a default body — any third-party implementation of these interfaces must add the method.

In-tree blast radius is 5 factory classes and 1 registry class, all under src/; there are no implementations in tests/, samples/ or benchmarks/, no derived classes, and the Func-based SimpleMessageMapperFactory constructor signature is unchanged, so its call sites keep compiling.

Note for direct resolvers of mappers/transforms. Because GetTransient now allocates and retains an IServiceScope per resolution regardless of whether the resolved instance is IDisposable (the scope can own the instance's disposable dependencies and its own injected IServiceProvider, so its lifetime must follow the instance, not the instance's disposability), any code that resolves a mapper or transform itself — via IAmAMessageMapperRegistry.Get<T>() or a factory Create — must now call Release (or ReleaseAsync) when finished, even for a non-disposable mapper such as the default JsonMessageMapper. All in-tree call sites already do. Skipping the release retains one empty scope until factory disposal.

Root cause

Part A. GetTransient<T> lazily created one IServiceScope for the factory's entire (process) lifetime and reused it for every transient resolution. The built-in DI container tracks each resolved IDisposable in the scope's internal disposables list; that reference is only removed when the scope itself is disposed. Release() called disposal.Dispose() on the individual instance but never disposed the scope, so the container retained one strong reference per message indefinitely. When the factory was eventually disposed the scope re-disposed every previously-released instance (double-dispose). Because TransformerLifetime defaults to Transient and transformer factories are process-lifetime singletons, this is the default code path.

Part B. After Part A, GetTransient<T> deliberately retains instance → scope for IDisposable instances on the contract that the owner calls Release. Transformers honour that contract via TransformLifetimeScope → IAmAMessageTransformerFactory.Release. Mappers could not: the mapper factory interfaces declared only Create, no implementation had a Release, and MessageMapperRegistry never released what Create returned. Release and Dispose are the only two drains of _transientScopes, so for mappers only the second was reachable — meaning a user-defined IDisposable mapper under the default MapperLifetime = Transient retained one IServiceScope per message until process shutdown, and was never disposed.

Two further defects surfaced while confirming that diagnosis, both of which had to be fixed for Part B to actually work:

• HasPipeline orphaned a mapper per message. It was return _mapperRegistry.Get<TRequest>() != null; — an instance created purely to answer a question, immediately unreachable, owned by nothing. OutboxProducerMediator calls it on every send and receive, so this was 2 mapper creations per outbound message and 3 on the sync-fallback path. With the DI defaults (DefaultMessageMapper = JsonMessageMapper<>, registered Transient) Get essentially never returns null, so the worst case was the normal case.
• ServiceProviderLifetimeScope.Release was not idempotent, and was unsafe under Scoped. It fell through to a direct disposal.Dispose() when no tracked transient scope was found, so a repeat release double-disposed. Under MapperLifetime = Scoped — a supported configuration — that fall-through disposed an instance that remains cached in _scopedInstances, so message No API Documentation #2 would receive a disposed mapper. Introducing a per-message mapper release would have activated this.

Fix

Part A. GetTransient creates a fresh IServiceScope per call and stores instance → scope in a ConcurrentDictionary keyed by reference identity. Release calls scope.Dispose() — the DI container drops its tracked reference and disposes the instance exactly once. Dispose drains any unreleased transient scopes before disposing the shared scope used for scoped resolutions.

Part B.

• Release added to both mapper factory interfaces and all five implementations: ServiceProviderMapperFactory / ...Async delegate to ServiceProviderLifetimeScope.Release; SimpleMessageMapperFactory / ...Async and ControlBusMessageMapperFactory are documented no-ops (the caller's Func owns what it returns and may hand back a shared instance; the control-bus mappers hold no resources). This mirrors SimpleMessageTransformerFactory, whose Release is likewise a no-op.
• Release / ReleaseAsync added to the registry interfaces and MessageMapperRegistry, forwarding to the matching factory.
• TransformPipeline / TransformPipelineAsync take an optional trailing mapperRegistry parameter and release the mapper on disposal, guarded by an Interlocked.Exchange flag so an explicit Dispose followed by another cannot double-release. The mapper is released outside InstanceScope, because that scope only exists when a transformer factory was supplied — releasing through it would miss the no-transformer case entirely.
• The four concrete pipelines take the registry as an optional trailing constructor parameter, so external direct construction is not source-broken.
• HasPipeline releases its probe in a finally. Its semantics are kept byte-for-byte — the tempting swap to the existing type-only MessageMapperRegistry.ResolveMapperInfo was not taken, because it is not equivalent to Get<TRequest>() != null: it ignores the null-factory guard, it reports a mapper that the container cannot actually resolve, and its IsGenericTypeDefinition branch turns an InvalidOperationException into a false. That refactor is tracked separately.
• OutboxProducerMediator (CreateRequestFromMessage, MapMessage, MapMessageAsync) now uses using var pipeline = …. Reactor and Proactor use using var pipelineLifetime = pipeline as IDisposable;, which works despite their reflective object? handle because TransformPipeline<T> implements IDisposable non-generically.
• ServiceProviderLifetimeScope.Release now returns early for any non-Transient lifetime and drains only via TryRemove, making it idempotent and a no-op under Scoped/Singleton.

Test plan

Part A:

• When_releasing_a_transient_transformer_the_factory_should_not_retain_it — sync factory, was red, now green
• When_releasing_a_transient_transformer_async_the_factory_should_not_retain_it — async factory, was red, now green

Part B — each was proved a genuine RED by temporarily reverting the single production hunk it pins:

• When_disposing_a_wrap_pipeline_should_release_the_transient_disposable_mapper — RED Expected: 1 / Actual: 0
• When_checking_for_a_pipeline_should_release_the_probe_mapper — RED Expected: 10 / Actual: 0
• When_posting_a_message_should_release_every_mapper_it_creates — drives a real CommandProcessor.Post over OutboxProducerMediator + InMemoryOutbox, asserting CreateCount == ReleaseCount with no forced GC.Collect (release must be deterministic, not finalizer-driven). RED Expected: 2 / Actual: 1 — precisely diagnostic: the probe mapper released, the pipeline's own mapper orphaned.

Suites, all green on both net9.0 and net10.0:

Project	Result
Paramore.Brighter.Core.Tests	873 passed / 7 skipped
Paramore.Brighter.Extensions.Tests	124 passed
Paramore.Brighter.InMemory.Tests	144 passed
Paramore.Brighter.Testing.Tests	85 passed
Paramore.Brighter.AsyncAPI.Tests	46 passed
Paramore.Brighter.Transforms.Adaptors.Tests	30 passed

Not run locally: the transport/backend integration suites, which need a container runtime. They exercise this path through CommandProcessor, so they are worth a CI run, though the change is transport-agnostic.

Update — async release so pump disposal cannot deadlock (review item 1)

Review item 1 was a genuine deadlock, not only a stall. The async pipeline is disposed on the thread that releases it; on the Proactor that is the message-pump thread, running under the single-threaded BrighterSynchronizationContext. Releasing a transient IAsyncDisposable mapper/transform ran through ServiceProviderLifetimeScope.DisposeScope's blocking pending.AsTask().GetAwaiter().GetResult(). A user DisposeAsync that awaits without ConfigureAwait(false) posts its continuation back to the pump context — which the blocked pump thread can never drain. Hard deadlock.

Fixed with a true async release path plus a defensive offload.

Part A — genuine async release through the async pipeline

• TransformPipelineAsync<T> now implements IAsyncDisposable. DisposeAsync awaits async release of the transforms (TransformLifetimeScopeAsync.DisposeAsync) and the mapper (IAmAMessageMapperRegistryAsync.ReleaseAsync), so an IAsyncDisposable disposal is awaited, not blocked on — the pump thread stays free to run any continuation posted back to its context. The synchronous Dispose/finalizer remains the fallback, sharing the Interlocked release-once guard.
• New ValueTask ReleaseAsync on IAmAMessageMapperFactoryAsync, IAmAMessageTransformerFactoryAsync, and IAmAMessageMapperRegistryAsync; ServiceProviderLifetimeScope.ReleaseAsync shares its bookkeeping with Release via a new CollectScopesToRelease. No-op factories return default.
• Proactor.TranslateMessage and OutboxProducerMediator.MapMessageAsync now await using. The synchronous CreateRequestFromMessage stays on using by design (it is already sync-over-async).

Part B — deadlock-safe synchronous fallback

• ServiceProviderLifetimeScope.DisposeScope now runs the disposal on a pool thread when a SynchronizationContext is current (continuations resume off-pump), and keeps the cheap inline path when none is (Reactor, finalizer, factory shutdown — unchanged, no per-release cost there). This downgrades any residual deadlock on the synchronous fallback to a bounded stall.

Breaking change (extends the list above). The registry's ReleaseAsync added earlier in this PR was void; it is renamed to Release and a genuine ValueTask ReleaseAsync added — so no additional break over released Brighter for the mapper registry/factory. IAmAMessageTransformerFactoryAsync does gain a new ValueTask ReleaseAsync(IAmAMessageTransformAsync). On netstandard2.0 there are no default interface members, so a third-party implementation of these interfaces must add the method. In-tree blast radius is the four async factories, the registry, and the async transform test doubles; there are no other implementers.

Test. When_releasing_an_async_disposable_mapper_on_the_pump_context_it_should_not_deadlock drives both disposal shapes on a real BrighterAsyncContext pump with an IAsyncDisposable mapper whose DisposeAsync yields without ConfigureAwait(false). Proven RED by reverting Part B's offload — the synchronous-dispose case then deadlocks (15s Join timeout, exact diagnostic) while the await using case still passes, showing A and B each prevent it independently. Hosted on a dedicated thread with a thread-pool floor so it does not flake under parallel load.

Update — finalizers made non-throwing so a failed release cannot crash the process (review item 2)

Review item 2 flagged that an IAsyncDisposable-only mapper/transform reaching ~TransformPipeline() → Release → DisposeScope → the synchronous scope.Dispose() throws InvalidOperationException on the finalizer thread (MS DI's sync scope Dispose throws for an async-only service), and an exception escaping a finalizer terminates the process — an escalation from a throw to a hard teardown. Both pipeline finalizers released with no guard, so the same crash also applied to any user Dispose/DisposeAsync that threw.

Fix. ~TransformPipeline() and ~TransformPipelineAsync() now wrap their ReleaseUnmanagedResources() call in try/catch and swallow, honouring the .NET invariant that a finalizer must never throw. Explicit Dispose/DisposeAsync is unchanged and still surfaces the exception to the caller that owns the pipeline; only the finalizer fallback swallows. This closes the process-crash escalation; the broader "handle IAsyncDisposable-only instances on the synchronous Dispose path" change remains the tracked follow-up (see out of scope below).

Test. When_a_pipeline_finalizer_release_throws_it_should_not_escape (two facts, sync WrapPipeline and async WrapPipelineAsync) abandons an undisposed pipeline whose registry Release throws and forces finalization via GC.Collect + WaitForPendingFinalizers. Proven RED by reverting the sync guard — the test host crashes with Unhandled exception … at TransformPipeline.Finalize() rather than failing an assertion, since an escaping finalizer exception tears down the process. Green with the guard; full Paramore.Brighter.Core.Tests 873 passed / 7 skipped on net9.0 and net10.0; production build clean on all four TFMs including netstandard2.0.

Update — round 5 review follow-ups (lifetime-scope finalizer guards, symmetrical async no-op release, reply-path deadlock test)

Answering the round-5 review (comment). The scope-churn note on the outbox hot path (removing the HasPipeline/ResolveMapperInfo probe allocation) is deferred with the reviewer's agreement — flagged out of scope below.

Lifetime-scope finalizers made non-throwing. The prior round hardened the two pipeline finalizers; the same crash-on-finalizer hazard applied to the lifetime scopes they own. ~TransformLifetimeScope() and ~TransformLifetimeScopeAsync() called ReleaseTrackedObjects() bare, so releasing an IAsyncDisposable-only transform through the synchronous path (MS DI's sync scope Dispose throws for an async-only service) could let an exception escape the finalizer and terminate the process — and finalization order is non-deterministic, so an orphaned scope finalized before its owning pipeline could take that path. Both finalizers now wrap the release in try/catch and swallow, matching the pipeline-finalizer rationale. Explicit Dispose/DisposeAsync stays unguarded and still surfaces to the owner. Test: When_a_transform_lifetime_scope_finalizer_release_throws_it_should_not_escape (two facts, sync + async), each proven RED by reverting its guard (host crash at TransformLifetimeScope[Async].Finalize()).

Symmetrical async no-op release. EmptyMessageTransformerFactoryAsync.ReleaseAsync called sync Dispose(); it now awaits DisposeAsync() when the transform is IAsyncDisposable, else falls back to sync Dispose(), matching the reader factory's shape. Behaviour-preserving (EmptyMessageTransformAsync only implements IDisposable today); covered by existing suites staying green.

Reply-path sync-over-async deadlock test (test-only). OutboxProducerMediator.CreateRequestFromMessage builds an async unwrap pipeline but disposes it via a synchronous using and blocks the unwrap on GetAwaiter().GetResult(); on the Proactor pump its deadlock-safety rides on the DisposeScope offload. The round-3 deadlock test covered only the send path (Proactor.TranslateMessage); this adds the distinct reply-path shape, driving CreateRequestFromMessage on a real BrighterAsyncContext pump with an IAsyncDisposable mapper. Proven RED by disabling the DisposeScope offload (30s Join timeout). The reply path releases the mapper twice — a throwaway HasPipeline<TRequest> probe mapper plus the pipeline's own — so the test asserts == 2, with a comment naming both sources so that whoever removes the probe allocation (the deferred hot-path change) knows why the expected count drops to 1. Both pump-context deadlock tests are placed in a DisableParallelization xUnit collection: each blocks a dedicated pump thread on an offloaded disposal that needs a free pool thread, and letting them contend with the whole parallel suite starved that offload and tripped the guard spuriously. The sync-over-async unwrap itself is the accepted, pre-existing reply-path design and is left unchanged.

Mapper released even when transform-scope disposal throws. A follow-up review note caught a residual instance of the very leak this PR closes. All three disposal paths (TransformPipeline.ReleaseUnmanagedResources, and TransformPipelineAsync's DisposeAsync and ReleaseUnmanagedResources) released the mapper after disposing the transform scope (InstanceScope), with the release-once guard (_released) already set at the top. So a throw from transform-scope disposal — the synchronous-scope-throw shape guarded against elsewhere — skipped the mapper release, and because the guard was set neither the finalizer nor a later dispose retried it, permanently orphaning the mapper's DI scope. Fix: release the mapper in a finally in all three paths, so scope-disposal failure can no longer orphan it; the scope-disposal exception still surfaces to an explicit Dispose/DisposeAsync caller (only the finalizer swallows). Test: When_a_transform_scope_disposal_throws_the_mapper_is_still_released (three facts, one per disposal path), each proved RED by reverting the try/finally in the hunk it pins.

Verified green on both net9.0 and net10.0: Paramore.Brighter.Core.Tests (878 passed / 7 skipped) and Paramore.Brighter.Extensions.Tests (125 passed, stable across repeated runs). Production build clean on all TFMs including netstandard2.0.

Update — round 7 review follow-up (atomic first-scope publish; sync-fallback offload confirmed inherent)

Answering the latest review (comment).

First scoped resolution made race-safe (review item 1). ServiceProviderLifetimeScope.GetOrCreateScoped published the shared scope with a non-atomic _scope ??= _serviceProvider.CreateScope(). Two threads racing the first scoped resolution each CreateScope(); the loser's assignment is overwritten and never disposed, because neither Dispose nor Release drains a scope that is not the retained _scope — leaking one empty scope per lifetime scope under concurrent first-use of a Scoped mapper/handler. Fix: publish via Interlocked.CompareExchange(ref _scope, created, null) and have the loser dispose the scope it created. Test: When_two_threads_first_resolve_a_scoped_mapper_concurrently_it_should_not_leak_a_scope rendezvouses the first two CreateScope() calls on a barrier so the race is forced (asserts created == 2), then asserts created == disposed after factory disposal. Proven RED against the non-atomic assignment (disposed stayed at 1 while created was 2).

Sync-fallback offload (review item 2) — confirmed inherent, no change. The note that the DisposeScope synchronous fallback (Task.Run(...).GetAwaiter().GetResult() when a SynchronizationContext is current) consumes a pool thread under load is correct, and it is the unavoidable cost of draining an IAsyncDisposable-only scope through a synchronous release contract: the choice on that path is block-with-offload or deadlock, and the offload is what avoids the deadlock. The primary ReleaseAsync / await using paths avoid it entirely, and the pump-deadlock tests add a thread-pool floor, so it is a bounded stall rather than a hang. Left as-is by design.

Verified green on both net9.0 and net10.0: Paramore.Brighter.Core.Tests (878 passed / 7 skipped) and Paramore.Brighter.Extensions.Tests (126 passed — the new race test added). Production build clean on all TFMs including netstandard2.0.

Update — round 8 review follow-up (second full-diff review: four fixes + handler-scope semantics pinned)

Answering the second full-diff review (part 1, part 2). Five new/decision items: four fixed, one investigated and confirmed correct (then pinned). Each fix keeps this PR's discipline — a RED test proven by reverting the pinned production hunk, then green on net9.0 + net10.0.

Ambiguous Release overload for a dual-interface mapper (part 1, item 1 🔴). MessageMapperRegistry exposed both Release(IAmAMessageMapper) and Release(IAmAMessageMapperAsync) publicly. Any mapper supporting both Reactor and Proactor implements both markers (e.g. JsonMessageMapper<T>), so registry.Release(mapper) on the concrete type failed to compile with CS0121. In-tree callers dodged it via interface-typed fields, but the concrete registry is public and the PR now asks callers to "Release what you Get". Fix: implement the async Release explicitly (void IAmAMessageMapperRegistryAsync.Release), leaving only the synchronous Release on the concrete surface; async release stays available via the interface or ReleaseAsync. RED-proven: the added test did not compile before the change.

GetOrCreateScoped create-vs-Dispose race (part 1, item 2 🟠). The symmetric case to round 7: a thread can pass the disposed-guard, create its first scoped scope, and publish it via CompareExchange after Dispose has already drained _scope (still null), orphaning it. Fix mirrors GetTransient: after winning the publish CAS, re-check _disposed and reclaim the published scope with a second CAS; Dispose now claims _scope with Interlocked.Exchange so exactly one side disposes it (also collapsing the check-then-use double-read flagged as a nit); the scoped Lazy factory throws ObjectDisposedException rather than dereferencing a nulled _scope. RED-proven: the published scope's DisposedCount stayed at 0.

Transform lifetime scope partial-release / retry double-dispose (part 1, item 3 🟠). TransformLifetimeScope[Async].ReleaseTrackedObjects[Async] released with a foreach over the unmodified list, so a throw on one transform skipped the rest, and the finalizer retry re-released the ones that had already succeeded (a double dispose for a user factory whose Release disposes). Fix: drain-as-you-go (remove each transform before releasing it) in all three loops. RED-proven: the retry released the first transform twice and the last never.

Release bookkeeping abandoned on DisposeScope throw (part 2, item 5 🟡). CollectScopesToRelease was a lazy iterator, so a DisposeScope throw in the caller's foreach unwound it before the key-removal step, leaving the now-empty stack keyed by the instance — the same instance-keyed retention this PR closes, resurfacing on the failure path. Fix: materialise the bookkeeping eagerly into a list before any scope is disposed. RED-proven: the instance stayed tracked after a throwing release.

Handler-scope semantic change (part 2, still-open item) — investigated, confirmed correct, pinned. The reviewer flagged that changing GetTransient from a shared _scope to a per-resolution scope alters the default (Transient) handler path: a scoped dependency injected into a handler and its middleware used to be one shared instance and is now distinct per handler. Investigation confirms this is the correct lifetime split, not a regression. The leak lived only in the mapper/transformer factories, whose ServiceProviderLifetimeScope is app-lifetime (so transients accumulated); the handler factory's scope is per-pipeline and disposed wholesale at chain end, so it never leaked. Transient means each resolution gets its own independent scope; Scoped means the scope is the call chain, shared across handlers — and GetOrCreateScoped still shares _scope, so the Scoped unit-of-work / transaction-provider pattern (#3754) is preserved. No production change; added HandlerLifetimeCallChainScopeTests pinning both semantics across a two-handler chain (the Transient-isolation fact would fail on master, documenting the intended default-path behavior).

Not changed: the DisposeScope context-suppression optimization (part 2, item 4 🟡) — a throughput improvement rather than a leak fix, deferred with the other hot-path follow-ups below.

Verified green on both net9.0 and net10.0: Paramore.Brighter.Core.Tests (881 passed / 7 skipped) and Paramore.Brighter.Extensions.Tests (130 passed). Production build clean on all TFMs including netstandard2.0.

Deliberately out of scope

Adjacent defects found while confirming the diagnosis, each recorded in bugfixes/0007-mapper-factory-release-disposable-mapper/bugfix.md:

• IAsyncDisposable-only instances currently throw on the synchronous scope-disposal path at ServiceProviderLifetimeScope.cs. The guard added in Part A is instance is IDisposable, so an IAsyncDisposable-only instance falls through to a synchronous scope disposal, and MS DI's ServiceProviderEngineScope.Dispose() throws when it holds such a service. This affects transforms in the current code, not just mappers. The finalizer-thread process-crash half of this is now fixed (review item 2 above); handling async-only instances correctly on the synchronous Dispose path — rather than swallowing — remains to be done and deserves its own fix.
• MessageMapperRegistry.ResolveMapperInfo's IsDefault flag goes stale after the first Get, affecting TransformPipelineBuilder.DescribeTransforms.
• ServiceProviderHandlerFactory.Release disposes the handler directly and then disposes the lifetime scope, double-disposing transients.
• The ResolveMapperInfo refactor that would remove the HasPipeline probe allocation entirely — needs its own behavioural analysis for the three divergences above.

Fixes #4252

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254: dispose per-message IServiceScope on Release

Thorough diagnosis (the bugfix.md write-up is excellent) and the core fix for the transformer path is correct: creating a fresh scope per Create, tracking instance → scope by reference identity, and disposing that scope in Release cleanly closes both the unbounded-growth and double-dispose defects. The two regression tests pin the sync and async transformer factories well, and the InstanceComparer using ReferenceEquals/RuntimeHelpers.GetHashCode is a nice defensive touch (it avoids relying on a user transform overriding Equals/GetHashCode).

However, there is one issue I would consider a blocker, plus a few smaller notes.

Blocker — the fix introduces a new unbounded leak on the mapper path

ServiceProviderLifetimeScope is shared by all four factories, but only the two transformer factories have a Release method. ServiceProviderMapperFactory and ServiceProviderMapperFactoryAsync expose only Create and Dispose — there is no Release.

With the new GetTransient, every Create now (1) allocates a fresh IServiceScope, and (2) stores instance -> scope in _transientScopes.

Because mappers are never released, _transientScopes grows by one entry (mapper instance + IServiceScope) per message and is only drained at factory Dispose (process end). MapperLifetime defaults to Transient (BrighterOptions.cs:35), the mapper factory is a process-lifetime singleton, and MessageMapperRegistry.Get<T>() calls _messageMapperFactory.Create(...) on every pipeline build with no caching (MessageMapperRegistry.cs:87,109) — so this fires for every message in every Brighter app using the DI integration with defaults.

This is strictly worse than before: previously mappers reused a single scope and, not being IDisposable, nothing accumulated (the mapper leak was latent/unobservable). After this change the mapper path leaks the retained instances and the scope objects. Your own bugfix.md Scope Note (a) called this out precisely:

"A per-instance-scope fix for mappers has nowhere to dispose the scope (no Release hook) — so mapper scopes would accumulate ... unless the fix either avoids allocating a scope when nothing disposable is tracked, or gives mappers a release path."

The applied fix does not do either. Options to close it:

• Give the mapper factories a Release path (as the transformer factories have), wired through the mapper registry/pipeline teardown; or
• In GetTransient, avoid retaining a scope when the resolved instance is not IDisposable/IAsyncDisposable (dispose the scope immediately in that case) — cheaper and self-contained, though it will not help a future disposable mapper.

There is also currently no test covering the mapper factories, which is exactly why this regression is not caught. Adding mapper coverage (mirroring the transformer tests) would guard it.

Minor / worth noting

• Per-message CreateScope() cost. Each transform (and mapper) resolution now allocates a full DI scope per message where it previously shared one. This is the right trade-off to fix the leak (and mirrors ServiceProviderHandlerFactory), but it is a real per-message allocation increase worth being aware of for high-throughput pipelines.
• Double-Release of a transient falls through to direct dispose. If Release(instance) is called twice for the same transient, the second call's TryRemove fails and execution falls through to disposal.Dispose() (ServiceProviderLifetimeScope.cs:142-143), disposing an already-disposed instance. Edge case / caller misuse, but the transient branch could return unconditionally to be safe.
• Scoped path retains a pre-existing double-dispose. For Scoped lifetime, Release disposes the instance directly while Dispose also disposes _scope (which re-disposes the container-tracked scoped instance). This predates the PR and is not the default, so it is out of scope — flagging only for awareness.
• _scope ??= _serviceProvider.CreateScope() in GetOrCreateScoped is not atomic under concurrent first-use (could create and leak a second scope). Also pre-existing; low risk given usage.

Summary

The transformer fix is correct and well-tested. The blocker is that the change is applied to the shared lifetime scope, so it converts the previously-benign mapper path into an active per-message leak on the default configuration. I would resolve the mapper scope-retention (and add mapper test coverage) before merging.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254 (transient transformer scope leak)

Thanks for the thorough diagnosis in bugfix.md — the root-cause analysis is excellent and the transformer fix itself is sound. The instance → scope tracking with reference-identity keying, disposing the scope in Release, and draining leftovers in Dispose correctly resolves the double-dispose / unbounded-retention defect on the transformer path, and the two regression tests pin it well.

However, this change appears to trade the transformer leak for a new, equally unbounded leak on the mapper path — arguably a more common hot path than transforms.

🔴 Blocking: new unbounded leak in the mapper factories

GetTransient<T> is shared by all four factories. It now creates a fresh IServiceScope per call and stores instance → scope in _transientScopes, relying on Release to remove and dispose the entry.

The transformer factories call _lifetimeScope.Release(...) (via TransformLifetimeScope.ReleaseTrackedObjects), so their entries are drained. The mapper factories never do:

• ServiceProviderMapperFactory / ServiceProviderMapperFactoryAsync expose only Create + Dispose — no Release method, and nothing in the codebase calls one (grep confirms no mapper release path).
• MessageMapperRegistry.Get<TRequest>() / GetAsync<TRequest>() call _messageMapperFactory.Create(...) on every message (they cache the mapper type, not the instance — MessageMapperRegistry.cs:87,109).
• Default MapperLifetime is Transient (BrighterOptions.cs:35) → GetOrCreate → GetTransient.

Net effect: every message now permanently retains one IServiceScope object plus a strong reference to the mapper instance (the dictionary key), cleared only at process-lifetime factory Dispose. Before this PR the mapper factory reused a single shared _scope and — because IAmAMessageMapper(Async) is not IDisposable — retained nothing per message. So this is a genuine regression: an unbounded, per-message leak on the default mapping path.

Your own bugfix.md Scope Note (a) predicted exactly this:

"a per-instance-scope fix for mappers has nowhere to dispose the scope (no Release hook) — so mapper scopes would accumulate empty IServiceScope objects unless the fix either avoids allocating a scope when nothing disposable is tracked, or gives mappers a release path."

The shipped fix does neither. Suggested options:

1. Only retain the scope when something disposable is tracked — after resolving, keep the scope only if instance is IDisposable/IAsyncDisposable; otherwise dispose the child scope immediately. Non-disposable mappers then never accumulate. Smallest change, keeps mappers allocation-neutral.
2. Give mappers a Release path (larger; touches IAmAMessageMapperFactory).

🟠 Minor / worth noting

• Test coverage gap: the regression is on the mapper factory, but there are no mapper tests, and no test asserts actual retention/growth (only the dispose-count proxy on transforms). A mapper-factory test would have caught the issue above.
• Scoped-lifetime transformer double-dispose (pre-existing, adjacent): for ServiceLifetime.Scoped the instance is captured by _scope and Release calls disposal.Dispose() directly, then Dispose() disposes _scope → disposed twice. Non-default and pre-existing, but the GetOrCreateScoped refactor sits right here.
• Create/Dispose race: GetTransient/Release don't check _disposed and Dispose isn't synchronized. A Create racing Dispose can insert into _transientScopes after the drain loop, leaking that scope. A _disposed guard would be cheap insurance.
• Duplicate-instance overwrite: if a transform/mapper is (mis)registered as a singleton while lifetime is Transient, repeated Create returns the same instance and _transientScopes[instance] = scope overwrites the prior scope, leaking it until Dispose. Edge case, but the reference-identity key assumes uniqueness.
• Nit: DisposeCountingTransform.Context is public IRequestContext Context (non-nullable) while the interface is IRequestContext? — a nullable-annotation warning in test code.

Reviewed with Claude Code.

[claude]

Code Review — PR #4254: dispose per-message IServiceScope on Release

Thanks for the detailed write-up — the root-cause analysis in the PR body and the two bugfix.md files make this easy to follow, and the fix targets the real defect: GetTransient<T> used to reuse one process-lifetime IServiceScope, so the built-in DI container retained (and later double-disposed) every transient IDisposable transform. Moving to a per-call scope keyed by instance identity and disposing it in Release is the right shape. A few observations, none of them blockers.

Correctness / design

1. is IDisposable guard leaves a documented latent leak for disposable mappers. Commit 3 changes the store guard from instance != null to instance is IDisposable (ServiceProviderLifetimeScope.cs:119). That correctly stops non-disposable mappers from accumulating scopes. But mapper factories still expose no Release method and MessageMapperRegistry never releases, so a user-defined IDisposable mapper under the default Transient lifetime would still pile up one scope per message until factory shutdown. bugfix.md (0006) calls this out as "Part B" (add Release to the mapper factories/registry) and it is explicitly not implemented here. Thats a reasonable scope cut, but it means the underlying asymmetry (transformers have a release path, mappers dont) is only papered over for the common case. Recommend tracking Part B as a follow-up issue rather than leaving it in a bugfix doc, since its a real (if narrow) leak.

2. Double-Release of the same transient falls through to a direct Dispose(). In Release (:137-145), if a transient disposable instance is released twice, the second call misses TryRemove and drops into if (instance is IDisposable disposal) disposal.Dispose() — disposing the instance directly a second time. TransformLifetimeScope only releases each tracked object once so this shouldnt happen in-tree, and MS DI disposables are generally idempotent, but the fall-through is slightly surprising given the methods stated intent of "dispose exactly once." Consider having the transient branch return unconditionally once _lifetime == Transient, rather than falling through to the generic disposal path.

3. Minor thread-safety notes (pre-existing pattern, not worsened): _scope ??= _serviceProvider.CreateScope() in GetOrCreateScoped (:102) is not atomic; a concurrent first-touch could create and leak a second scope (same pattern existed before via GetTransient, so not a regression). And Dispose() iterates _transientScopes.Values while a concurrent Release could TryRemove+dispose the same scope — ConcurrentDictionary enumeration is safe and scope disposal is idempotent, but neither Release nor Dispose check _disposed. Fine in practice given the factory lifecycle, worth a comment.

Test coverage

4. The transformer leak tests can pass even if Release disposes nothing. Both When_releasing_a_transient_transformer[_async]_the_factory_should_not_retain_it assert Assert.Equal(disposeCountAfterRelease, transform.DisposeCount) — i.e. that the count doesnt change across factory.Dispose(). That correctly catches the double-dispose/retention bug, but it would also pass if the instance were never disposed at all (0 == 0). Adding Assert.Equal(1, disposeCountAfterRelease) would pin the "disposed exactly once on Release" behavior and close that gap.

5. The accumulation test is strong — ScopeTracker/TrackingServiceProvider counting real scope disposals (no reflection) and asserting 10 == 10 after 10 Creates is a clean, robust regression guard for the mapper path. Nice.

Style / conventions

6. Matches the repos DI factory patterns and the When_... test naming convention. XML docs on the changed members were updated to reflect the new semantics. InstanceComparer using RuntimeHelpers.GetHashCode + ReferenceEquals is the correct choice for identity keying.

Summary

Solid, well-scoped fix for the reported leak with good regression coverage. Main things Id want before/after merge: a tracked follow-up for the disposable-mapper case (Part B), and ideally the stronger Assert.Equal(1, ...) on the transformer tests. Nothing here blocks the core fix for #4252.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254: dispose per-message IServiceScope on Release

Thorough, well-documented fix. The root-cause analysis in bugfixes/000{5,6,7} is excellent, and the change is internally consistent: per-instance scope tracking, symmetric Release on the mapper side, deterministic pipeline disposal at the call sites, and idempotent release guards (Interlocked.Exchange in TransformPipeline, TryRemove in ServiceProviderLifetimeScope). I verified all in-tree implementors of the changed interfaces were updated (Simple*, ControlBus*, ServiceProvider*, MessageMapperRegistry) and that sync/async paths mirror each other. Nice work.

A few things worth considering before merge:

1. (Correctness — please double-check) Non-disposable mapper with disposable dependencies may get a use-after-dispose

In ServiceProviderLifetimeScope.GetTransient<T>:

```csharp
var scope = _serviceProvider.CreateScope();
var instance = (T?)scope.ServiceProvider.GetService(objectType);
if (instance is IDisposable)
_transientScopes[instance] = scope;
else
scope.Dispose(); // <-- disposes the scope immediately
```

When the resolved instance is not IDisposable (which is every in-tree mapper), the scope is disposed immediately. The built-in container captures every IDisposable created while resolving within that scope — including a mapper's transient/scoped dependencies. So a non-disposable mapper that injects a disposable dependency (e.g. a claim-check store, a crypto/serializer service, a repository) will have that dependency disposed before the pipeline calls MapToMessage/MapToRequest on it → potential ObjectDisposedException.

Before this PR that couldn't happen (the single shared scope leaked but kept dependencies alive). Now that mappers do have a Release path (Part B), the is IDisposable short-circuit looks like it's no longer buying much and reintroduces this risk. Options: track all transient instances and let Release dispose the scope (dependencies then stay valid until the pipeline is disposed), or explicitly document that IoC-resolved mappers must not depend on disposable scoped services. Worth a targeted test either way — the current suite only covers non-disposable mappers with no dependencies.

2. (Perf / follow-up) HasPipeline still instantiates a mapper per operation

OutboxProducerMediator calls HasPipeline<TRequest>() on every send and receive, and HasPipeline now does Get<TRequest>() + Release(...) purely to null-check — creating (and, for a disposable mapper, scoping/disposing) a full instance on the hot path, plus another for the actual Build*Pipeline. The type-only ResolveMapperInfo/ResolveAsyncMapperInfo helpers already exist (and bugfix 0006 scope-note (a) flags exactly this) — routing HasPipeline through them would avoid the allocation entirely. Not blocking, but it's a per-message cost the current approach leaves on the table.

3. (API / SemVer) Source-breaking interface change

Adding Release to the public IAmAMessageMapperFactory{,Async} and IAmAMessageMapperRegistry{,Async} is source-breaking for any external implementer (netstandard2.0 rules out a default interface member). The blast radius in-tree is fully enumerated and handled, and these are documented DI extension points, so it's defensible — but it should be called out in release notes and reflected in the version bump (this isn't a patch-level change).

Minor

• Process artifacts under bugfixes/** and the .current-bug pointer (now 0007-...) are committed as part of the PR. If intended, fine — but .current-bug is transient working state and might be better cleared.
• Async disposal timing in Proactor.TranslateMessage is correct — the Task is awaited inside the using scope before it exits. Good.

Test coverage

Good: create/release balance (When_posting_a_message_should_release_every_mapper_it_creates), transient retention (sync + async), non-disposable no-accumulation, HasPipeline probe release, and wrap-pipeline disposable release. The main gap is item #1 above (mapper with a disposable dependency).

Automated review — verify findings independently; I could not run a build in this environment.

[iancooper]

Two notes on the latest two commits (2ba4437c8, 47958ab33) that aren't obvious from the diff.

1. This PR fixes a regression it introduced itself

2d5a9c1c5 (bugfix 0006, earlier on this branch) broke 6 of 12 Paramore.Brighter.Validation.FluentValidation.Tests. Bisected:

Commit	Result
c73e93221 (before the series)	12 passed
e6a6d4aa0	12 passed
2d5a9c1c5 — "dispose transient scope immediately for non-IDisposable instances"	6 failed

ObjectDisposedException : Cannot access a disposed object. Object name: 'IServiceProvider'.
  at FluentValidationRequestHandler`1.Validate(TRequest) in FluentValidationRequestHandler.cs:line 52

0006's guard lives in ServiceProviderLifetimeScope, which is shared by the handler, mapper and transformer factories — so it also eagerly disposed handler scopes, and handlers have had a working Release all along. FluentValidationRequestHandler injects IServiceProvider and resolves from it lazily, so it got a disposed provider.

The guard's premise — "mappers have no Release" — was dissolved by this PR's Part B, which gave mappers one. A DI scope owns more than its instance; it owns whatever the instance captured from it, so scope lifetime has to follow instance lifetime, not disposability. GetTransient is back to instance != null, with Release bounding accumulation instead.

Consequence: 0006's regression test was deliberately rewritten. It asserted 10 Create calls → 10 disposals with no Release, which is exactly the reverted premise. It now drives Create+Release pairs, plus a new fact pinning that the scope must survive until Release.

Full reasoning and evidence: bugfixes/0007-mapper-factory-release-disposable-mapper/bugfix.md, follow-up (c). 0006's own bugfix.md now carries a warning that its Part A was reverted.

2. One deliberate behaviour change — worth a release note

Follow-up (h): create-after-dispose on a DI factory now throws ObjectDisposedException. It previously succeeded silently and leaked the scope. Nothing in the suite depended on the old behaviour, but it is a visible contract change for anyone calling Create on a disposed factory.

Also folded in

Seven follow-ups total, (b)–(h) in 0007's bugfix.md — the IAsyncDisposable-only throw, the Build*Pipeline exception-path mapper leak, the Scoped-path async-disposal gap, the stale ResolveMapperInfo.IsDefault flag (which also made Register throw "already has a mapper" if Get ran first), and the ServiceProviderHandlerFactory double-dispose (which affected Scoped as well as Transient).

Verification

Both net9.0 and net10.0, all green: Core 865 (was 863) · Extensions 121 (was 117) · InMemory 144 · Testing 85 · AsyncAPI 46 · Transforms.Adaptors 30 · FluentValidation 12 (was 6/12) · DataAnnotations 10.

Paramore.Brighter, the DI extensions and ServiceActivator build clean on all four TFMs including netstandard2.0 — the only proof available for the #if NETSTANDARD2_0 branch, since test TFMs are net9/net10 only.

Not run: transport/backend suites needing a container runtime.

[claude]

Code Review — Release per-message transformers and mappers to their factories

I read through the full change (DI lifetime scope, mapper factory/registry surface, the four transform pipelines + builders, and the five production call sites) against the root-cause writeup and the new tests. Overall this is a high-quality, carefully reasoned fix. The diagnosis in the PR body matches the code, the sync/async paths are symmetric, the netstandard2.0 constraints are respected, and the tests are RED-proven rather than finalizer-driven. A few observations below, all fairly minor.

Strengths

• ServiceProviderLifetimeScope is the crux and it's solid: per-call scope keyed by reference identity, Release gated on Transient + TryRemove (idempotent, safe under Scoped/Singleton), and the _disposed/re-check dance in GetTransient is a genuinely careful piece of shutdown-ordering reasoning.
• Deterministic disposal everywhere — using var pipeline in OutboxProducerMediator, using var pipelineLifetime = pipeline as IDisposable in Reactor/Proactor — instead of leaning on ~TransformPipeline(). The Interlocked.Exchange(ref _released, …) guard correctly makes Dispose+finalizer idempotent.
• HasPipeline probe release in a finally, with the explicit decision not to swap to ResolveMapperInfo (documented divergences) — the conservative call is the right one here.
• MessageMapperRegistry splitting _resolvedDefaultMappers out of the registration dictionaries (follow-up f) is a clean fix that also repairs the Register-after-Get throw and the stale IsDefault.
• Tests are meaningful: When_posting_a_message_should_release_every_mapper_it_creates drives real CommandProcessor.Post and asserts CreateCount == ReleaseCount with no forced GC.Collect.

Points worth considering

1. Partial-failure transform leak in the builders (low). In BuildWrapPipeline/BuildUnwrapPipeline (both sync and async), if BuildTransformPipeline throws after creating one or more transforms — e.g. a later CreateMessageTransformer() returns null and raises InvalidOperationException — the catch releases the mapper but the already-created transforms are never released (the pipeline that would own them via InstanceScope was never constructed). This is a narrow path, but since the PR is precisely about closing release gaps it's worth either releasing the partial transform list in the catch or noting it as knowingly out of scope.

2. _transientScopes[instance] = scope key collision (low/edge). Under MapperLifetime = Transient but a mapper registered as a singleton in the container, GetService returns the same instance for every call, so the indexer assignment overwrites the prior entry and that earlier IServiceScope is never disposed — one leaked (empty) scope per message, and Release drains only the last. It's a misconfiguration, but a TryAdd-and-dispose-the-loser (or a documented assumption that Transient resolutions are distinct) would make it defensive rather than silently leaky.

3. Sync-over-async DisposeScope on the message-pump thread (info). Proactor disposes the async pipeline synchronously via using var (IDisposable), so an IAsyncDisposable-only mapper is drained through DisposeScope's GetAwaiter().GetResult() on the pump thread. The remark documents this ("completes inline unless a user's DisposeAsync performs real I/O"), which is the correct tradeoff for the sync release contract — just flagging that a user mapper doing real async I/O in DisposeAsync would block the pump here.

4. GetTransient vs Dispose enumeration race (very low). If a Create races a concurrent Dispose such that Dispose's foreach (…Values) observes the just-added entry (disposing that scope) and Clear() then removes it, the post-add TryRemove(instance,…) returns false, so GetTransient returns an instance whose scope is already disposed without throwing. This requires concurrent create-during-dispose (already documented as misuse) and is astronomically unlikely; noting only for completeness.

None of these block the fix — the core leak/double-dispose defects (#4252 and the Part B mapper path) are convincingly closed and well-tested. Items 1–2 are the ones I'd most want a line of code or a comment on.

Note: I could not run the build/tests in this review environment; the assessment is from reading the diff against the stated test results.

🤖 Generated with Claude Code

[iancooper]

Thanks for the review — round 2 pushed (b72d72c15, f41288a61, e970a1635). Addressing the four "worth considering" points:

1 — Partial transform build leak ✅ fixed (b72d72c15)
BuildTransformPipeline created transforms in a loop with no try/catch; if a later transform failed to be created or to Initialize, the already-created ones never reached a pipeline's TransformLifetimeScope and were never released to the transformer factory. Closed all three release-gap windows (sync + async): mid-loop failure now releases every already-created transform before rethrowing; a build that fails after construction disposes the constructed pipeline in the outer catch; and TransformerFactory[.Async] releases a transform whose Initialize throws. 6 new regression tests.

2 — Transient scope key collision ✅ fixed (f41288a61)
GetTransient tracked each transient IServiceScope in a dictionary keyed by the resolved instance (reference equality) via an indexer set. When the same reference came back twice — a type registered AddSingleton but resolved under a Transient MapperLifetime (reachable because mappers/transforms register TryAdd(..., Transient)) — the second store overwrote the first, orphaning that scope; neither Release nor Dispose could reclaim it, and it's unbounded because the factories hold one process-wide lifetime scope.

Note on the fix: the suggested TryAdd + dispose-the-loser approach turned out to be only a partial fix — disposing the redundant scope eagerly would use-after-dispose a shared-disposable transient (AddTransient(sp => sharedDisposable)), where the loser scope actually owns the instance. Instead I key by a ConcurrentStack<IServiceScope> per instance: GetTransient pushes, Release pops+disposes one (detaching the key when empty), Dispose drains all. Symmetric create/release, no leak on collision, disposal stays deferred so nothing is disposed while still in use. New regression test asserts scopes-created == scopes-disposed.

3 — Sync-over-async DisposeScope on the pump thread ✅ documented (e970a1635)
This is the correct tradeoff for the synchronous release contract, so no code change — but I've documented it: an IAsyncDisposable-only mapper/transform drains synchronously through DisposeScope's awaited DisposeAsync, on the Proactor's message-pump thread, so a DisposeAsync doing real async I/O would block the pump. Added author guidance on DisposeScope warning against real I/O in a mapper/transform DisposeAsync, cross-referenced from Release.

4 — GetTransient vs Dispose enumeration race — left as-is
Requires concurrent create-during-dispose (already documented as misuse) and is astronomically unlikely; the existing _disposed post-add re-check already covers the realistic window. Happy to revisit if you'd prefer it hardened.

Full Paramore.Brighter.Extensions.Tests green 122/122 on net9.0 and net10.0; the transform tests green across Core.Tests both TFMs.

[claude]

Code review — release per-message transformers and mappers to their factories

Reviewed the full diff against CLAUDE.md conventions. This is an unusually well-executed fix: the root-cause analysis is precise, every RED was proven by reverting the single hunk that pins it, the breaking change is scoped and justified, and back-compat is preserved by making the new constructor/registry parameters optional. The concurrency reasoning in ServiceProviderLifetimeScope (stack-per-instance, Interlocked release-once, reference-identity comparer to avoid relying on mapper Equals/GetHashCode) is careful and matches the inline commentary. Below is feedback, roughly in priority order.

Correctness / robustness

1. Sync-over-async disposal is broader than the pump warning suggests (ServiceProviderLifetimeScope.DisposeScope). The pending.AsTask().GetAwaiter().GetResult() path blocks the releasing thread for the whole DisposeAsync, and it runs on more threads than the Proactor pump. TransformPipeline/TransformPipelineAsync are IDisposable (not IAsyncDisposable), so even in OutboxProducerMediator.MapMessageAsync the using var pipeline disposes synchronously on the continuation thread. If a caller drives CommandProcessor.Post/Publish from a context with a SynchronizationContext (classic ASP.NET, a UI thread) and a mapper/transform is IAsyncDisposable-only, a DisposeAsync that resumes on the captured context can deadlock — not just stall. The doc guidance ("no real I/O in DisposeAsync") mitigates the common case, but it's advisory and the failure mode for a misbehaving implementation is a hang, not a leak. Worth either (a) broadening the XML warning beyond the pump to "any thread with a SynchronizationContext", or (b) tracking a genuine async release path as a follow-up.

2. Narrow race in Release's re-drain (ServiceProviderLifetimeScope.Release, the IsEmpty && Remove(...) block). If a concurrent GetTransient pushes onto the same stack in the window between the emptiness check and the KVP Remove, the re-drain disposes the just-pushed scope while its instance has already been handed back to that caller — the caller then holds an instance whose scope's IServiceProvider is disposed, and (since _disposed is false) its post-push re-check won't catch it. This is only reachable when the same instance is resolved transiently concurrently with its own release (i.e. a shared/singleton-under-Transient registration). Very low likelihood, and the alternative (leak) was reasonably rejected — but the comment claims the re-drain "keeps that scope from being orphaned" without noting it can dispose an in-use scope. A one-line acknowledgement would keep the reasoning honest.

3. Finalizer now releases the mapper too (TransformPipeline.ReleaseUnmanagedResources → _mapperRegistry?.Release). On the leak/no-Dispose fallback, a user mapper's Dispose/DisposeAsync now runs on the finalizer thread (previously only transforms did). The five production call sites all use using, so this is genuinely a fallback, but a mapper doing heavy work in Dispose could stall process-wide finalization. Acceptable as-is; noting for awareness.

Performance

4. HasPipeline + Build*Pipeline still creates two mappers per outbound message. The PR body already calls this out and defers the ResolveMapperInfo-based probe (the divergences you list — null-factory guard, IsGenericTypeDefinition branch — are real reasons not to swap it in blindly). Just confirming the extra allocation is understood and deferred, not overlooked; the important half (both are now released) is done.

Minor / nits

5. MessageMapperRegistry.Get<T> caches MakeGenericType in _resolvedDefaultMappers, but ResolveMapperInfo recomputes it each call. Harmless (the latter isn't on the per-message path), but slightly inconsistent now that the two caches are split.

6. ControlBusMessageMapperFactory.cs is still missing a trailing newline (pre-existing, but the diff touches the last line).

Test coverage

Strong. The RED-proven regression tests, the Post-driven end-to-end CreateCount == ReleaseCount assertion with no GC.Collect (proving deterministic release rather than finalizer-driven), the partial-build/initialize-throws leak windows, the scoped/async-disposable variants, and the key-collision case together cover the surface area well. The FluentValidation regression (6/12 → 12/12) that surfaced and reverted the instance is IDisposable guard is exactly the kind of thing an integration-level test should catch — good that it did.

Nothing here is blocking; items 1 and 2 are the ones worth a decision (fix vs. document-and-defer) before merge. Given the change is transport-agnostic but flows through CommandProcessor, a CI run of the transport/backend suites (not run locally per the PR) is worthwhile.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254: release per-message transformers and mappers to their factories

Reviewed the core of the change: ServiceProviderLifetimeScope, the TransformPipeline/TransformPipelineAsync base classes and the four concrete pipelines, MessageMapperRegistry, both pipeline builders, OutboxProducerMediator, Reactor/Proactor, and the factory/interface additions. This is a high-quality, unusually well-reasoned fix — the diagnosis (single long-lived scope → retained DI references + double-dispose) is correct, the race handling in GetTransient/CollectScopesToRelease/Dispose is carefully thought through, and the test coverage is genuinely diagnostic (each RED was proven by reverting the single hunk it pins). The stacked-scope handling for a shared instance resolved under a Transient lifetime (InstanceComparer + ConcurrentStack) is the right call and avoids the obvious overwrite-and-orphan trap.

A few things worth confirming before merge.

1. Contract change is now stricter than the leak it fixes (worth a migration note)

Previously, resolving a non-disposable transient mapper and dropping it leaked nothing — MS DI does not track non-disposables, and there was a single shared scope. After this change, every transient resolution allocates an IServiceScope stored in _transientScopes until Release, so any caller that obtains a mapper via IAmAMessageMapperRegistry.Get<T>() / factory Create and does not call Release now retains an (empty) scope until factory disposal — even for the default JsonMessageMapper. All in-tree call sites are covered (verified: only the two pipeline builders resolve mappers, and both release). But this is a real behavioural tightening for any third-party/plugin code that calls Get/Create directly. Combined with the netstandard2.0 interface break, this deserves an explicit line in the upgrade/release notes: "if you resolve mappers or transforms yourself, you must now Release them."

2. IAsyncDisposable-only instances via the finalizer can escalate to a process crash

The PR correctly flags (out of scope) that ServiceProviderLifetimeScope sync disposal throws for IAsyncDisposable-only services. One case is nastier than the others: an un-disposed pipeline reaches ~TransformPipeline() → ReleaseUnmanagedResources() → Release → DisposeScope → scope.Dispose(), which throws InvalidOperationException on the finalizer thread. An exception escaping a finalizer tears down the process. This already affects transforms in current code (as noted), but the elevated severity — crash, not just a throw — argues for pulling the IAsyncDisposable-only fix forward rather than leaving it purely as a follow-up. At minimum, DisposeScope on the finalizer path should swallow/log rather than let it propagate.

3. Performance: extra scope + double mapper creation per message

Two cost increases on the hot path, both correctness-driven and both acknowledged, but worth a throughput sanity check:

• GetTransient now does _serviceProvider.CreateScope() per resolution (was one shared scope for the process).
• HasPipeline still creates a probe mapper and the subsequent Build*Pipeline creates a second — 2 mapper creations (+2 scopes) per outbound message on the default path, 3 on the sync-fallback path. The PR notes the ResolveMapperInfo refactor that removes the probe is tracked separately; given this is the default code path, it would be reassuring to see a before/after benchmark (or at least confirmation the integration/throughput suites were run in CI) rather than shipping the allocation regression and optimising later.

4. Minor: PR description drift

The description says Proactor uses using var pipelineLifetime = pipeline as IDisposable;, but the code correctly uses await using var pipelineLifetime = pipeline as IAsyncDisposable; (the right choice on the pump). Similarly the async registry is described as gaining only ReleaseAsync, but Release(IAmAMessageMapperAsync) is added to IAmAMessageMapperRegistryAsync too. Just align the prose.

Things that are correct and I specifically checked

• Release/CollectScopesToRelease returns early for non-Transient lifetimes, so a shared Scoped/Singleton mapper is not disposed per message and stays cached — the earlier fall-through double-dispose is genuinely fixed, and idempotent double-release is a safe no-op (TryPop).
• Release after Dispose is a safe no-op (drained dict → TryGetValue miss), so finalizer ordering at shutdown will not throw for the common non-async-disposable case.
• The _disposed re-check after GetOrAdd/Push closes the create-vs-dispose race without orphaning the just-pushed scope; TryPop guarantees each scope disposes exactly once even if Dispose also sees the entry.
• Build-failure cleanup (CleanUpAfterFailedBuild / ReleaseTransforms) and the pipeline's own dispose are mutually exclusive, so no double-release on the error path.
• Breaking-change blast radius matches the claim: only the 5 in-tree factories + MessageMapperRegistry implement the changed interfaces (plus the new test double, which implements Release); no samples//benchmarks/ implementers.

Overall: solid, well-tested, and the design reasoning in the XML docs is excellent. My only merge-blocking-ish concern is deciding whether the finalizer-path IAsyncDisposable-only crash (#2) is acceptable to defer given it can take down a process. The rest are notes/nits.

🤖 Generated with Claude Code

[claude]

Code Review — PR 4254 (per-message transformer and mapper release)

Reviewed against CLAUDE.md / .agent_instructions. Overall this is an exemplary bug fix: the root-cause writeup, the RED-then-green proof for every production hunk, the documented breaking-change blast radius, and the out-of-scope tracking in bugfixes/0007-…/bugfix.md are exactly the diagnosis-first discipline the repo asks for. The concurrency reasoning in ServiceProviderLifetimeScope (per-instance scope stacking, the post-Push disposed re-check, the ICollection.Remove(KVP) compare-and-remove in CollectScopesToRelease) is careful and, as far as I can trace it, correct. The async deadlock analysis (item 1) and its SynchronizationContext-aware offload in DisposeScope are the highlight.

A few observations, mostly minor — none look like blockers:

1. HasPipeline now creates and disposes a DI scope on every send (perf, hot path). TransformPipelineBuilder.HasPipeline / …Async.HasPipeline resolve a real mapper (Get/GetAsync) then Release it. With the Part A change, a transient resolution is now a fresh IServiceScope per call — so on the default Transient lifetime, OutboxProducerMediator allocates+disposes a scope purely to answer "is there a pipeline?" on every Post/receive, on top of the pipeline own mapper. The PR flags this and deliberately defers the ResolveMapperInfo swap (with a sound rationale for why the two are not equivalent) — I agree with deferring, just want the throughput cost on the outbox hot path called out explicitly for reviewers/CI, since Part A converts what used to be scope reuse into scope churn here.

2. TransformLifetimeScopeAsync finalizer is not throw-guarded, unlike the pipeline finalizers you hardened. ~TransformLifetimeScopeAsync() calls ReleaseTrackedObjects() bare, whereas TransformPipeline/TransformPipelineAsync finalizers now wrap ReleaseUnmanagedResources() in try/catch precisely because a sync release of an IAsyncDisposable-only instance can throw (MS DI sync scope Dispose). This finalizer pre-dates the PR, so it is not a regression — but finalization order is non-deterministic, so an orphaned lifetime scope finalized before its owning pipeline finalizer runs Dispose on it would take the unguarded path and could crash the process. Since you are already touching this file and adding the async release path, mirroring the swallow-in-finalizer guard here would make the hardening consistent. (Same pattern likely applies to the sync TransformLifetimeScope.)

3. EmptyMessageTransformerFactoryAsync.ReleaseAsync calls sync Dispose(). It does transformer.Dispose(); return default;. Harmless for the trivial EmptyMessageTransformAsync, but it defeats the async intent of the new method — if it is meant to be the async null-object, awaiting DisposeAsync() (when available) would be more faithful. Cosmetic.

4. CreateRequestFromMessage — sync-over-async disposal of an async pipeline. It builds _transformPipelineBuilderAsync.BuildUnwrapPipeline but disposes via sync using and blocks with GetAwaiter().GetResult(). Documented as intentional (already sync-over-async), and the DisposeScope offload makes it deadlock-safe when a SynchronizationContext is current. The new deadlock test exercises the pump path via TranslateMessage; it would be worth confirming a test also covers this sync-over-async reply path with an IAsyncDisposable mapper, since it is a distinct call shape relying on the same offload.

Test coverage. Strong — each Part B fix pinned by a test proven RED against a reverted hunk, CreateCount == ReleaseCount asserted with no GC.Collect (deterministic release, not finalizer-driven), and the deadlock test proven RED by reverting the offload. The integration/transport suites (not runnable here without a container) are the one gap, and you have correctly noted they are worth a CI run even though the change is transport-agnostic.

Nice work — the depth of the writeup made this reviewable despite the 67-file surface.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254: release per-message transformers and mappers to their factories

Reviewed against the repository conventions in CLAUDE.md. Overall this is a high-quality, deeply-reasoned fix. The root-cause analysis is precise, every fix is pinned by a RED-proven test, the concurrency and finalizer edges are handled with care, and the PR body itself is exemplary documentation. Feedback below is mostly about surfacing consequences, not defects — I did not find a correctness bug in the changed code.

Strengths

• Genuine root-cause fixes, not symptom patches. Part A (scope-per-transient tracked by reference identity, drained in Release) and Part B (mappers gaining a real release path) address the actual DI-scope retention, and the double-dispose / Scoped fall-through defects found along the way are fixed too.
• Tests prove causation, not just correctness. Each production hunk was reverted to confirm RED (Expected: 2 / Actual: 1 on the post path is precisely diagnostic), and When_posting_a_message_should_release_every_mapper_it_creates deliberately asserts determinism with no GC.Collect. That is exactly the right discipline.
• The deadlock analysis (review item 1) is correct and non-obvious — the single-threaded BrighterSynchronizationContext + a user DisposeAsync without ConfigureAwait(false) is a real hazard, and the true async release path plus the pool-thread offload each independently prevent it.
• Finalizer safety (review item 2) correctly honours the .NET invariant that a finalizer must never throw, while keeping explicit Dispose/DisposeAsync surfacing the exception to the owner.
• Concurrency in ServiceProviderLifetimeScope (stacked scopes per shared instance, the post-Push _disposed re-check, idempotent CollectScopesToRelease via TryPop/exact-stack Remove) is subtle but sound.

Points worth surfacing (not blockers)

1. Semver: this is a hard breaking change and should gate a major version. Adding non-defaulted members to four public interfaces on netstandard2.0 (no default interface members) source-breaks every third-party implementer of IAmAMessageMapperFactory(Async), IAmAMessageMapperRegistry(Async), and now IAmAMessageTransformerFactoryAsync. The PR documents this thoroughly — just make sure the release is versioned accordingly and the migration note lands in the changelog, not only the PR body.

2. Behavior change for direct resolvers is the biggest real-world footgun. Because GetTransient now retains an IServiceScope per resolution regardless of disposability, any external code calling the public IAmAMessageMapperRegistry.Get<T>() / factory Create directly will now accumulate one empty scope per call until factory disposal unless it calls Release. Previously a non-disposable mapper needed no cleanup. This is correct by design, but it is a silent semantic change for existing user code that compiles unchanged — worth calling out prominently in upgrade docs, not just the "Note for direct resolvers" aside.

3. HasPipeline still creates+releases a mapper on every send/receive (hot path). The probe allocation is retained (the ResolveMapperInfo refactor is correctly deferred), so OutboxProducerMediator pays a create+release per message. On the async path, releasing an IAsyncDisposable mapper via the sync _mapperRegistryAsync.Release(...) inside HasPipeline can trigger the DisposeScope pool-thread offload and block the pump thread for the disposal duration. Deadlock-safe, but a per-message throughput cost on the pump. Given the tracked follow-up, consider prioritising the ResolveMapperInfo-based probe so the hot path stops instantiating a mapper purely to answer a boolean.

4. await using var pipelineLifetime = pipeline as IAsyncDisposable; (Proactor.TranslateMessage) assumes a non-null pipeline. If MakeUnwrapPipeline ever returned null the as yields null (await-using-null is safe), but the very next line pipeline!.GetType() would NRE. This matches existing behavior (the reflection path always returns a pipeline), so it is pre-existing — but the ! is load-bearing, and a defensive null check throwing a clear MessageMappingException would read better than a raw NRE if that invariant is ever broken.

Test coverage

Strong. The added tests cover: transient non-disposable non-accumulation, double-resolve-before-release, scoped async-disposable-only disposal, the pump-context deadlock (both using and await using shapes on a real BrighterAsyncContext), and non-throwing finalizers for both pipeline and lifetime-scope. The explicit "reverted the hunk to confirm RED" methodology for each is exactly what the mandatory TDD workflow in CLAUDE.md is after. The one gap is honest and acknowledged: transport/integration suites were not run locally (need a container runtime) — a CI run over the CommandProcessor path is the right validation.

Nice work — a model example of diagnosis-first bugfixing with the adjacent defects tracked rather than silently folded in.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4254 (transient transformer/mapper release paths)

Thanks for a very carefully constructed PR — the inline documentation, the concurrency reasoning in ServiceProviderLifetimeScope, the finalizer-safety notes, and the breaking-change analysis are all excellent, and the test coverage (deadlock-on-pump, finalizer-throw, double-release, key-collision, partial-build cleanup) is genuinely impressive. I reviewed the full diff statically; I did not run the build or test suite, so please treat the compile/behaviour notes as things to confirm in CI.

🔴 Primary concern — scope-sharing semantics change for transient handlers (likely beyond the stated scope)

ServiceProviderLifetimeScope.GetTransient<T> no longer shares a single _scope; it creates a fresh IServiceScope per resolution. That is correct and necessary for the mapper/transformer leak this PR targets — but the same method also backs ServiceProviderHandlerFactory, and that side effect isn't mentioned in the PR description.

Before this PR, every transient handler resolved for one IAmALifetime shared a single DI scope (old GetTransient: _scope ??= _serviceProvider.CreateScope()). So a Scoped-registered dependency (a DbContext, a unit-of-work, etc.) was the same instance across the whole handler chain for one message/request. After this PR each transient handler gets its own scope, so those scoped dependencies are no longer shared across the pipeline.

This is a behavioural change that could break the common "one unit-of-work per message" pattern, and it goes beyond the stated fix. Could you confirm it's intended? If it is, it would be worth an explicit test asserting the intended sharing (or non-sharing) semantics for a Scoped dependency resolved by two handlers in the same pipeline, plus a CHANGELOG note — the handler path is hotter and more widely relied on than mappers/transforms. If it isn't intended, the transient-scope change probably needs to be confined to the mapper/transformer factories rather than applied to every GetTransient consumer.

🟠 Performance — extra IServiceScope allocations per message

Every transient resolution now allocates and retains an IServiceScope (plus a ConcurrentStack entry) until Release. For a pipeline that resolves several handlers, plus the mapper, plus each transform — and the extra probe mapper that HasPipeline now creates and releases — that is several scope allocations/disposals per message on the framework's hottest path, where the old code allocated one shared scope. Worth a run through the benchmarks/ project to confirm no meaningful steady-state regression.

🟡 Minor / edge cases

• TransformPipelineAsync.DisposeAsync leak on exceptional disposal: _released is set to 1 up front, but GC.SuppressFinalize(this) is the last statement. If InstanceScope.DisposeAsync() throws, the mapper is never released and the finalizer (still registered) no-ops because _released == 1, so the mapper leaks. Same shape in the sync ReleaseUnmanagedResources. Only reachable when disposal itself throws, so low priority — a try/finally around the release steps would close it.
• DisposeScope sync-over-async on the pump context (Task.Run(() => asyncScope.DisposeAsync().AsTask()).GetAwaiter().GetResult()): avoids the deadlock but still blocks the pump thread for the whole disposal. The comments acknowledge this and the ReleaseAsync/DisposeAsync path (which the Proactor correctly uses via await using) is the real mitigation — good; just flagging that the synchronous fallback is a genuine stall if a user's DisposeAsync does I/O, which the author-guidance comment also calls out. 👍
• Trailing newline: IAmAMessageMapperFactory.cs, SimpleMessageMapperFactory.cs, and ControlBusMessageMapperFactory.cs show "No newline at end of file" — nit.

✅ Things I checked and liked

• The _transientScopes stack-per-instance design (with InstanceComparer reference identity) and the disposal-race handling across GetTransient / CollectScopesToRelease / Dispose are carefully reasoned and look correct.
• Removing the explicit handler.Dispose() in ServiceProviderHandlerFactory.Release is right — scope disposal now disposes the handler exactly once, closing the double-dispose.
• The separate _resolvedDefaultMappers cache is a good latent-bug fix: resolving a default mapper no longer poisons _messageMappers, so ResolveMapperInfo keeps reporting IsDefault: true. (Also technically a behavioural fix beyond the stated scope — fine, but worth a line in the description.)
• Finalizers correctly swallow all exceptions with documented rationale; the Interlocked.Exchange release guard is correct and shared across Dispose/DisposeAsync/finalizer.
• I confirmed every in-tree implementer of the four changed interfaces (and IAmAMessageTransformerFactoryAsync) is updated in both src/ and the test doubles, so the breaking interface change shouldn't break tree compilation.

Overall this is high-quality, well-tested work. The one thing I'd want resolved before merge is confirmation (and ideally a test) on the transient-handler scope-sharing change in 🔴, since it's a behavioural change on a hot, widely-used path that the description doesn't call out.

[claude]

Review — PR #4254: mapper/transformer release & transient scope leak

Thorough, well-motivated change. The root-cause write-ups (both in the PR body and the bugfixes/*.md files) are excellent, the diagnosis is convincing, and each production hunk is pinned by a test proven RED first. Overall a solid fix. Comments below, grouped by theme — most are discussion/nits rather than blockers.

Strengths

• Correct root cause for ServiceProviderTransformerFactory(Async)/ServiceProviderMapperFactory(Async) never dispose their IServiceScope, causing unbounded memory growth for IDisposable transients #4252. Moving from one long-lived IServiceScope to a per-resolution scope tracked by reference identity, drained in Release, is the right model. The ConcurrentStack per instance (for a shared instance backing several scopes) and the _disposed re-check race handling in GetTransient/CollectScopesToRelease are carefully reasoned.
• Idempotency + Scoped safety. Making Release a no-op for non-Transient lifetimes and draining only via TryPop/TryRemove fixes the latent double-dispose and the "disposed instance handed to message No API Documentation #2 under Scoped" hazard. Good catch, and tested.
• Finalizers made non-throwing. Wrapping the release call in try/catch in all four finalizers honours the .NET invariant that a finalizer must not throw, while explicit Dispose/DisposeAsync still surface exceptions. The process-crash escalation was a real risk.
• Deadlock analysis. The genuine async release path (DisposeAsync → ReleaseAsync → DisposeScopeAsync) plus the pool-thread offload in DisposeScope when a SynchronizationContext is current is the correct fix for the pump-thread deadlock, and the dedicated deadlock tests (send + reply path, DisableParallelization) are convincing.
• Partial-build cleanup. Releasing transforms created before a later one fails, releasing after a failed Initialize* in TransformerFactory/TransformerFactoryAsync, and CleanUpAfterFailedBuild disposing the pipeline-or-parts exactly once — these close pre-existing leaks. The reasoning that transforms is only non-null in CleanUpAfterFailedBuild when BuildTransformPipeline returned successfully is correct.

Discussion / possible issues

1. Breaking change scope (highest-impact item). Adding non-default members to five public interfaces (IAmAMessageMapperFactory, IAmAMessageMapperFactoryAsync, IAmAMessageMapperRegistry, IAmAMessageMapperRegistryAsync, plus ReleaseAsync on IAmAMessageTransformerFactoryAsync) is a genuine source break on netstandard2.0 (no default interface members). The in-tree blast radius is documented well, but this warrants a major version bump and a prominent migration-guide entry — any third-party mapper factory or registry implementation (custom containers, downstream test doubles) will fail to compile. Worth confirming no transport package or wider ecosystem code implements these.

2. Mapper can leak if transform-scope disposal throws. In both TransformPipeline.ReleaseUnmanagedResources and TransformPipelineAsync.DisposeAsync/ReleaseUnmanagedResources, the mapper release runs after InstanceScope disposal:

InstanceScope?.Dispose();
_mapperRegistry?.Release(MessageMapper);   // skipped if the line above throws

Because _released is already 1, the finalizer's retry is a no-op, so a throw during transform disposal permanently orphans the mapper's scope — the exact leak class this PR closes. Consider releasing the mapper in a finally (best-effort) even when scope disposal throws. Low frequency, but it is a regression path the rest of the PR is careful to avoid.

3. Per-resolution scope allocation on the hot path (performance). GetTransient now allocates and retains an IServiceScope for every resolution regardless of disposability, and HasPipeline still creates+releases a probe mapper on every OutboxProducerMediator send/receive. That is ~2 scope create/dispose cycles per outbound message (3 on the sync-fallback reply path) under the default JsonMessageMapper/Transient config, where the shared scope previously amortised this. Removing the HasPipeline/ResolveMapperInfo probe is deferred with reviewer agreement — reasonable, but since the outbox is throughput-critical this deferral deserves a tracking issue with a benchmark, so the regression is measured rather than assumed acceptable.

4. DisposeScope offload cost + implicit contract. Task.Run(() => asyncScope.DisposeAsync().AsTask()).GetAwaiter().GetResult() when a SynchronizationContext is current means every synchronous release of an IAsyncDisposable scope on the pump incurs a thread-pool hop + blocking wait. Correct for deadlock avoidance, but the guidance ("a mapper/transform DisposeAsync should avoid real I/O") is an implicit performance contract on user code that is easy to violate unknowingly. Consider surfacing it in the public XML docs of the factory/registry ReleaseAsync members, not only internal remarks.

5. _resolvedDefaultMappers and ResolveMapperInfo. Splitting resolved default mapper types into _resolvedDefaultMappers/_resolvedDefaultAsyncMappers (so a default fallback no longer pollutes _messageMappers) is a good fix, tested by When_describing_a_default_mapper_after_use_should_still_identify_as_default. Confirm no other reader of _messageMappers relied on the old "default is promoted to registered after first Get" behaviour — the PR lists ResolveMapperInfo's stale IsDefault as out of scope, so it is worth double-checking the two do not interact.

Test coverage

Excellent — deterministic assertions (CreateCount == ReleaseCount with no forced GC.Collect), each production hunk proven RED by targeted revert, both disposal shapes exercised on a real pump context, and async-disposable-only edge cases covered. The DisableParallelization collection for the pump-deadlock tests is the right call. Suggestion: an explicit test for finding #2 (transform-scope disposal throws, mapper still released) would lock in that contract if you fix it.

Minor / nits

• ControlBusMessageMapperFactory.cs and IAmAMessageMapperFactory.cs are missing a trailing newline.
• The bugfixes/*.md working files (esp. the 824-line 0007-.../bugfix.md) are committed to the repo. Confirm that is intended — if they are durable design records that is fine, otherwise they add noise to the source tree.

Nice work overall — the correctness reasoning is unusually rigorous for a concurrency/lifetime fix.

Review generated with Claude Code.

[claude]

Code Review — PR #4254 (transient transformer/mapper release)

Reviewed against the base branch. This is a careful, high-quality fix for a genuine and subtle class of DI-scope/lifetime leaks. The diagnosis in the description is accurate, the code is unusually well-commented on the why, and the test coverage is excellent — each RED was independently proven by reverting the exact production hunk it pins, and the deterministic CreateCount == ReleaseCount assertion (no forced GC.Collect) is exactly the right way to prove release is owner-driven rather than finalizer-driven. Nice work.

Below are observations, ordered by significance. None are blockers; most were already acknowledged in the description as deferred/out-of-scope, and I'm confirming they're the right calls plus adding a couple of new ones.

Correctness / concurrency

1. GetOrCreateScoped — non-atomic _scope ??= _serviceProvider.CreateScope() (minor, pre-existing). ServiceProviderLifetimeScope.cs: two threads racing the first scoped resolution can each CreateScope(); the loser's scope is overwritten and never disposed (Dispose only drains the single _scope field and _transientScopes). This leaks at most one empty scope per lifetime-scope under concurrent first-use of a Scoped mapper/handler, and the same _scope ??= pattern existed before this PR (previously inside GetTransient), so it's not a regression. Worth a follow-up (Interlocked.CompareExchange and dispose the loser), not something to hold this PR for.

2. DisposeScope offload blocks a pool thread and the caller thread. The Task.Run(...).GetAwaiter().GetResult() path (when a SynchronizationContext is current) correctly avoids the deadlock, but under heavy Proactor concurrency it consumes a pool thread per synchronous release while the pump thread is also parked. The ReleaseAsync/await using paths (the primary paths) avoid this, and the tests add a thread-pool floor, so it's a bounded stall rather than a hang — just flagging that the synchronous fallback is not free under load.

Performance

3. Per-message scope churn on the default hot path. GetTransient now allocates a fresh IServiceScope (+ a ConcurrentDictionary/ConcurrentStack entry) on every resolution regardless of the instance's disposability — correct, since the scope can own the injected IServiceProvider — but with the DI defaults (JsonMessageMapper registered Transient) this is now several scope allocations per message: the pipeline mapper, each transform, plus the HasPipeline probe mapper (2–3× per outbound message per the description). The probe-allocation removal is already tracked as out-of-scope with the reviewer's agreement; I'd just emphasize this is the default configuration's steady-state cost, so the deferred ResolveMapperInfo follow-up is worth prioritizing for throughput-sensitive users.

API / breaking change

4. Four public interfaces gain members with no default body (netstandard2.0). Correctly identified and unavoidable on netstandard2.0. Please ensure this lands in a major-version bump and is called out in the release notes / migration guide, since any third-party IAmAMessageMapperFactory / IAmAMessageMapperFactoryAsync / IAmAMessageTransformerFactoryAsync / registry implementation will fail to compile until it adds the method. In-tree blast radius is fully covered — I confirmed there are no other implementers in tests/, samples/, or benchmarks/.

5. New resolve-side release contract. The note that direct callers of IAmAMessageMapperRegistry.Get<T>() / factory Create must now call Release even for non-disposable mappers is a real behavioral expectation change for anyone resolving mappers themselves. Good that it's documented on the interface XML docs; worth surfacing prominently in the changelog too.

Things done well (worth keeping)

• Release-once guard (Interlocked.Exchange) shared across Dispose/DisposeAsync/finalizer, with the mapper released in a finally so a transform-scope disposal throw can't orphan it — this closes the exact leak the PR targets and is verified by When_a_transform_scope_disposal_throws_the_mapper_is_still_released.
• Non-throwing finalizers on both pipelines and the lifetime scopes, honoring the "a finalizer must never throw" invariant while still surfacing exceptions to explicit Dispose/DisposeAsync callers.
• The _disposed volatile + post-GetOrAdd re-check in GetTransient correctly handles the dispose-during-resolution race, and CollectScopesToRelease pops-one-per-release with a re-drain on the concurrent-push window — the stacked-scopes-per-instance design (for shared instances resolved under Transient) is a nice touch that a 1:1 map would have gotten wrong.
• Reactor uses using ... as IDisposable, Proactor uses await using ... as IAsyncDisposable against the reflective object? handle — leveraging the non-generic interface implementation so TRequest needn't be known. Clean.

Overall: approve in spirit. The remaining items are the two already-tracked follow-ups (probe allocation removal; IAsyncDisposable-only instances on the synchronous path) plus the minor scoped-scope race in (1). Confirm the major-version/release-notes handling for the breaking change before merge.

Reviewed by Claude Code.

[iancooper]

Thanks for the review. Addressed the two actionable items in round 7 (b9dcfe22c):

Item 1 — GetOrCreateScoped first-scope race: fixed. The non-atomic _scope ??= CreateScope() is now Interlocked.CompareExchange(ref _scope, created, null), and the loser of the race disposes the scope it created so it can't be orphaned. Test When_two_threads_first_resolve_a_scoped_mapper_concurrently_it_should_not_leak_a_scope forces the race deterministically — a barrier rendezvouses the first two CreateScope() calls so both threads observe the null field and create (created == 2) — then asserts created == disposed after disposal. RED-proven against the old assignment (disposed stayed 1).

Item 2 — sync-fallback offload consuming a pool thread: confirmed inherent, left as-is. This is the unavoidable cost of draining an IAsyncDisposable-only scope through a synchronous release contract: on that path the choice is block-with-offload or deadlock, and the offload is what prevents the deadlock. The primary ReleaseAsync / await using paths avoid it, and the pump-deadlock tests add a thread-pool floor, so it's a bounded stall, not a hang. No change.

Items 3–5 (probe-allocation hot-path churn; the two API/breaking-change notes) remain as previously agreed: 3 is a tracked out-of-scope follow-up, 4/5 are the major-version bump + release-notes/migration-guide handling.

Verified green net9.0 + net10.0: Core.Tests 878/7-skipped, Extensions.Tests 126/126 (new race test added); production build clean incl. netstandard2.0.

[iancooper]

Just a general note: the changes to interfaces here are only implemented internally, afaik. They are public to allow alternative implementations, but if you have your own DI extensions for use with Brighter, I suspect that you would not balk at us breaking you, but would want to fix the memory leak issues. We do use SemVer, but have a pragmatic approach to avoid stalling most customers for whom this is an invisible, and welcome, bug fix.

For that reason: document in the release notes, but don't increment the major version.

[claude]

Code Review — PR #4254 (release per-message transformers and mappers to their factories) — part 1/2

Reviewed the full src/ diff statically against master (c73e932). I could not run the build or test suite here, so treat compile/behaviour claims as things to confirm locally.

This remains excellent work — the diagnosis chain, the RED-proof-per-hunk discipline, and the concurrency commentary in ServiceProviderLifetimeScope are unusually good. Rather than restate earlier rounds, below are five things I believe are new; part 2 covers still-open items and test coverage.

1. 🔴 MessageMapperRegistry.Release is ambiguous for any mapper implementing both interfaces — including the default JsonMessageMapper<T>

MessageMapperRegistry now exposes two Release overloads on unrelated parameter types:

• Release(IAmAMessageMapper) (MessageMapperRegistry.cs:128)
• Release(IAmAMessageMapperAsync) (MessageMapperRegistry.cs:142)

IAmAMessageMapper and IAmAMessageMapperAsync are unrelated marker interfaces (IAmAMessageMapper.cs:32, IAmAMessageMapperAsync.cs:35), and JsonMessageMapper<TRequest> implements both (MessageMappers/JsonMessageMapper.cs:19) — as does essentially every hand-written mapper supporting Reactor and Proactor.

So this should fail to compile with CS0121 (ambiguous call):

var registry = new MessageMapperRegistry(...);   // concrete type — public, widely used
JsonMessageMapper<MyCommand> mapper = ...;
registry.Release(mapper);                        // ambiguous: neither overload is better

In-tree call sites dodge it because _mapperRegistry is interface-typed (TransformPipelineBuilder.cs:51), and DescribeTransforms takes the concrete registry but never calls Release. But MessageMapperRegistry is public and commonly held concretely, so a user following the new "you must now Release what you Get" guidance with a dual-interface mapper hits a compile error whose only workaround is an explicit cast. Since this is already a documented breaking change, worth settling now: implement one explicitly (void IAmAMessageMapperRegistryAsync.Release(...)), or give the async one a distinct name.

2. 🟠 GetOrCreateScoped still leaks a scope on the create-vs-Dispose race — the symmetric case to the one round 7 fixed

Round 7 closed the two-threads-race-the-first-scope leak with Interlocked.CompareExchange. The Dispose race is still open, and it is the same shape you deliberately guarded against in GetTransient:

// ServiceProviderLifetimeScope.cs:82
ThrowIfDisposed();            // T1 passes: _disposed == false
// ... T2 runs Dispose() to completion: sets _disposed, drains transients,
//     sees _scope == null so disposes nothing ...
// ServiceProviderLifetimeScope.cs:114
if (_scope is null) {
    var created = _serviceProvider.CreateScope();
    Interlocked.CompareExchange(ref _scope, created, null);   // succeeds — _scope is still null
}

T1's scope is published after Dispose() has already run, so nothing ever disposes it — one IServiceScope and everything it holds leaks for the lifetime of the owning graph. GetTransient handles exactly this with a post-Push _disposed re-check (lines 160-166); GetOrCreateScoped needs the same after the CAS. Narrow window (shutdown concurrent with first scoped resolution), three-line fix, mirrors code already in the file.

Related nit: Dispose() is not atomic (if (_disposed) return; _disposed = true;, lines 339-343), so two concurrent disposals can both reach DisposeScope(_scope). Benign against MS DI (idempotent), but Interlocked.Exchange would make the intent explicit.

3. 🟠 TransformLifetimeScope[Async].ReleaseTrackedObjects has the exact leak you just fixed for the mapper

The round-5 follow-up moved the mapper release into a finally so a throw from transform-scope disposal cannot orphan it. The transform list itself did not get the same treatment:

// TransformLifetimeScope.cs:44
private void ReleaseTrackedObjects()
{
    _trackedObjects.Each(trackedItem => { _factory.Release(trackedItem); ... });
}

If Release throws on transform #1 — the documented IAsyncDisposable-only sync-scope-disposal throw, or a user Dispose — transforms #2..N are never released. Same in TransformLifetimeScopeAsync.ReleaseTrackedObjectsAsync (TransformLifetimeScopeAsync.cs:64).

The retry path makes it worse: Dispose() throws before GC.SuppressFinalize, so the finalizer runs ReleaseTrackedObjects() again over the unmodified list, re-releasing what already succeeded. Idempotent through ServiceProviderLifetimeScope.Release (TryPop), but a user IAmAMessageTransformerFactory whose Release does transform.Dispose() gets a double dispose — and #3..N still never get released, because #2 throws again.

Draining as you go fixes both halves:

while (_trackedObjects.Count > 0)
{
    var trackedItem = _trackedObjects[0];
    _trackedObjects.RemoveAt(0);      // remove first: a throw doesn't block the rest on retry
    _factory.Release(trackedItem);
}

(The symmetric point on TransformPipelineAsync.DisposeAsync is fine: when it throws, GC.SuppressFinalize is skipped but _released is already 1, so the finalizer correctly no-ops.)

[claude]

Code Review — PR #4254 — part 2/2

4. 🟡 The DisposeScope offload fires on every release under a SynchronizationContext, not just async-disposable ones

// ServiceProviderLifetimeScope.cs:294-303
if (scope is IAsyncDisposable asyncScope)
{
    if (SynchronizationContext.Current is not null)
    {
        Task.Run(() => asyncScope.DisposeAsync().AsTask()).GetAwaiter().GetResult();
        return;
    }

ServiceProviderEngineScope — what MS DI's CreateScope() returns — implements IAsyncDisposable unconditionally, so scope is IAsyncDisposable is always true for the default container. The offload therefore triggers on every synchronous transient release whenever a SynchronizationContext is current, regardless of whether anything in the scope is actually async-disposable. That is a thread-pool hop plus a blocking wait per release on the Proactor pump (including the HasPipeline probe, so ≥2 per message), and also on a WPF/WinForms UI thread calling CommandProcessor.Post, where before this PR there was no hop at all.

The prior round concluded the offload is inherent, and it is — for the genuinely async case. But you can keep the deadlock safety and skip the hop for the common synchronous case by suppressing the context rather than moving threads:

var saved = SynchronizationContext.Current;
if (saved is not null) SynchronizationContext.SetSynchronizationContext(null);
try
{
    var pending = asyncScope.DisposeAsync();
    if (pending.IsCompleted) pending.GetAwaiter().GetResult();
    else pending.AsTask().GetAwaiter().GetResult();
}
finally { if (saved is not null) SynchronizationContext.SetSynchronizationContext(saved); }

With the context nulled, a user DisposeAsync that awaits without ConfigureAwait(false) captures null and resumes on the pool — the deadlock is still prevented — but a scope with nothing async-disposable completes inline with zero hops. Worth trying against When_releasing_an_async_disposable_mapper_on_the_pump_context_it_should_not_deadlock, which should stay green.

5. 🟡 Release abandons its bookkeeping if DisposeScope throws

CollectScopesToRelease is a lazy iterator (ServiceProviderLifetimeScope.cs:234): the TryPop runs, control returns to the caller for DisposeScope(scope), and only then does the iterator reach the "stack is empty, remove the key" step. If DisposeScope throws — the documented async-only case — the foreach unwinds and the key is never removed. Because _transientScopes is keyed by the instance, that keeps a strong reference to the mapper/transform alive until factory disposal: the same retention class this PR closes, on the failure path. Materialising the scopes eagerly, or wrapping the loop in try/finally, would close it.

---

Still-open items from earlier rounds worth a decision before merge

• Transient handler scope sharing (raised in the round-6 comment; I don't see a response). I verified against master: the old GetTransient was _scope ??= _serviceProvider.CreateScope(), and ServiceProviderHandlerFactory keeps one ServiceProviderLifetimeScope per IAmALifetime (ServiceProviderHandlerFactory.cs:127). So under the default HandlerLifetime = Transient, every handler in one message's chain previously shared a single DI scope — an AddScoped<DbContext>() / unit-of-work dependency was one instance across the chain. After this PR each handler gets its own scope, so those dependencies are no longer shared. That is a real semantic change on the hottest, most widely relied-on path, it isn't mentioned in the description, and no test pins the intended semantics either way. If intended, an explicit test (two handlers in one pipeline, assert same-or-different Scoped instance) plus a changelog line would make it safe; if not, the per-resolution scope should be confined to the mapper/transformer factories.
• HasPipeline sync-releases its probe on the async path (TransformPipelineBuilderAsync.cs:173) — _mapperRegistryAsync.Release(...), the blocking path, on the pump thread, once per message. Item 4 would reduce the cost; removing the probe entirely is the tracked follow-up.
• Semver / migration notes — agreed with earlier rounds that this needs a major bump and a changelog entry for the "you must now Release what you Get" contract, not only the PR body.

Test coverage

Strong and genuinely diagnostic — the "reverted the hunk to confirm RED" methodology on every fix is exactly what CLAUDE.md asks for, and the pump-context deadlock tests, the finalizer-throw tests, and the forced first-resolution race (GatedScopeTracker's barrier is a nice touch) are all convincing. Gaps I'd add, matching the items above: handler-chain scoped-dependency semantics, TransformLifetimeScope partial-release failure, and the GetOrCreateScoped-vs-Dispose race. The transport/integration suites remain the honest untested surface; a CI run over CommandProcessor is the right validation.

Nice work overall — item 1 and the handler-scope question are the two I'd want settled before merge; 2–5 are small and self-contained.

🤖 Generated with Claude Code

[iancooper]

Thanks for the second full-diff review — thorough and on point. Addressed in round 8 (d04bd11…6190d0b, pushed):

• Item 1 🔴 (ambiguous Release) — confirmed CS0121 for a dual-interface mapper (e.g. JsonMessageMapper<T>) held via the concrete registry. Made the async Release an explicit interface implementation, so only the sync Release is on the concrete surface and registry.Release(mapper) binds unambiguously; async release stays available via the interface / ReleaseAsync. RED-proven (the test did not compile before).

• Item 2 🟠 (GetOrCreateScoped vs Dispose) — fixed by mirroring GetTransient: post-CAS _disposed re-check that reclaims the published scope, and Dispose now claims _scope with Interlocked.Exchange (also collapsing the check-then-use double-read nit). RED-proven (DisposedCount stayed 0).

• Item 3 🟠 (transform-list partial release / retry double-dispose) — drain-as-you-go in all three release loops. RED-proven (retry released the first twice, the last never).

• Item 5 🟡 (Release bookkeeping abandoned on throw) — CollectScopesToRelease now materialises the pop + key-removal eagerly before any disposal. RED-proven (instance stayed tracked after a throwing release).

• Handler-scope semantics (still-open) — investigated and I believe this is the correct lifetime split, not a regression. The leak lived only in the mapper/transformer factories (app-lifetime ServiceProviderLifetimeScope, so transients accumulated); the handler factory's scope is per-pipeline and disposed wholesale at chain end, so it never leaked. Transient → each resolution gets its own independent scope; Scoped → the scope is the call chain, shared across handlers — and GetOrCreateScoped still shares _scope, so the Scoped transaction-provider / unit-of-work pattern (Transaction Provider Scope #3754) is preserved. Pinned both with HandlerLifetimeCallChainScopeTests across a two-handler chain; the Transient-isolation fact would fail on master, which documents that the default-path change is intended. Happy to reconsider if you'd rather confine the per-resolution scope to the mapper/transformer factories instead.

• Item 4 🟡 (DisposeScope context-suppression) — agree it's a real hot-path saving, but it's a throughput optimization rather than a leak fix, so I've left it with the other hot-path follow-ups (probe-allocation removal) rather than widen this PR.

Verified green on net9.0 + net10.0: Core.Tests 881/7-skip, Extensions.Tests 130/130; production build clean incl. netstandard2.0. PR body has a matching round-8 section.

[claude]

Code Review — PR #4254 (round 9)

Reviewed the current head fresh rather than re-treading the eight prior rounds. The core design reads well: per-resolution IServiceScope tracked by reference identity, a scope stack per instance so a shared registration can't orphan an earlier scope, a genuine ReleaseAsync path so the Proactor pump can await disposal, non-throwing finalizers. The CompareExchange first-scope publish in GetOrCreateScoped and the drain-as-you-go loops in TransformLifetimeScope[Async] both read as correct.

Four items below. One is a new leak of the same class this PR exists to close; one is a test-fidelity gap meaning several scope-accounting tests don't exercise the branch production takes. Caveat: I could not build or run the suite in this environment, so these come from reading the diff against master — item 1 in particular deserves a red test before you take my word for it.

---

1. 🟠 GetTransient orphans the scope when resolution throws

ServiceProviderLifetimeScope.cs:164-177:

var scope = _serviceProvider.CreateScope();
var instance = (T?)scope.ServiceProvider.GetService(objectType);   // <-- can throw
if (instance == null) { DisposeScope(scope); return null; }
var scopes = _transientScopes.GetOrAdd(instance, ...);
scopes.Push(scope);

If GetService throws — or the (T?) cast throws InvalidCastException — the scope exists but is neither tracked in _transientScopes nor disposed. Nothing can reclaim it: Release has no key to find, and Dispose only drains _transientScopes and _scope. ServiceProviderEngineScope has no finalizer, so any disposable dependency the container already resolved into that scope before the failure leaks undisposed, permanently.

This is new with this PR. On master, GetTransient was _scope ??= CreateScope() over one shared scope, so a throwing resolution created nothing extra.

Not an exotic path: GetService throws (does not return null) for the most common DI misconfiguration there is — the mapper/transform/handler is registered but a constructor dependency isn't:

services.AddTransient<MyMapper>();   // MyMapper(ISomethingUnregistered dep)
// -> InvalidOperationException: Unable to resolve service for type ... while attempting to activate MyMapper

MapperLifetime/TransformerLifetime default to Transient and those factories are process-lifetime singletons, so a Proactor pumping against a misconfigured mapper leaks one scope per message — this PR's own failure mode, on the failure path. It also reaches the handler factory (default HandlerLifetime = Transient), where the orphan survives ReleaseLifetimeScope.

Suggested fix, mirroring the existing null branch:

var scope = _serviceProvider.CreateScope();
T? instance;
try { instance = (T?)scope.ServiceProvider.GetService(objectType); }
catch { DisposeScope(scope); throw; }
if (instance == null) { DisposeScope(scope); return null; }

Existing coverage stops at the null case (FactoryErrorHandlingTests.Factory_UnregisteredHandler_ReturnsNull); a red test wants a registered type with an unregistered constructor dependency, asserting created == disposed on the scope tracker.

2. 🟡 Failed-build cleanup isn't drain-as-you-go, and can swallow the original error

Round 8 fixed this exact shape in TransformLifetimeScope[Async].ReleaseTrackedObjects, but the sibling on the partial-build path didn't get it. TransformPipelineBuilder.cs:223 / TransformPipelineBuilderAsync.cs:220:

transforms.Each(transform => _messageTransformerFactory.Release(transform));

A throwing Release on transform i skips i+1..n. Unlike the lifetime-scope case there's no retry to fall back on — no pipeline was constructed, so these transforms sit in no InstanceScope and no finalizer will ever see them; their DI scopes leak permanently. The same while (count > 0) { take [0]; RemoveAt(0); Release; } drain fixes it (over a materialised list).

Relatedly, TransformPipelineBuilder.cs:120/158 and the async twins:

catch (Exception e)
{
    CleanUpAfterFailedBuild(pipeline, transforms, messageMapper);   // may throw
    throw new ConfigurationException("Error building wrap pipeline ...", e);
}

This PR deliberately lets Release/Dispose surface exceptions to explicit callers, so cleanup here can throw — and if it does, the original e (the configuration error the user needs to see) is discarded in favour of a disposal failure. Worth try { CleanUpAfterFailedBuild(...); } catch { /* must not mask the build error */ }, or attaching it to Data.

[claude]

Code Review — PR #4254 (round 9), part 2

Continued from part 1.

3. 🟡 The scope test doubles are sync-only, so the accounting tests take a branch production never takes

DisposeScope branches on scope is IAsyncDisposable. Real MS DI scopes always satisfy that — ServiceProviderEngineScope has implemented IAsyncDisposable since .NET Core 3.0, and IServiceProvider.CreateScope() returns it. Every IServiceScope double in the new tests implements only IServiceScope/IDisposable:

• When_creating_transient_non_disposable_mappers_...:97 TrackingScope
• When_two_threads_first_resolve_a_scoped_mapper_concurrently_...:88 TrackingScope
• When_a_release_scope_disposal_throws_...:70 ThrowingDisposeScope
• plus the doubles in When_the_same_transient_mapper_is_resolved_twice_..., When_a_scope_is_first_published_while_the_owner_is_disposing_..., When_building_a_pipeline_throws_...

So all fall through to scope.Dispose(), while production on net8/9/10 always goes through DisposeAsync() — and on a pump thread, through the Task.Run(...).GetAwaiter().GetResult() offload. The bookkeeping assertions stay valid, but the branch that actually runs in a real app is unverified by them. Adding IAsyncDisposable to TrackingScope (counting either disposal shape) closes the gap cheaply and gives the offload path real coverage.

Two knock-ons from the same fact, both about comments rather than code:

• Roughly eight comments justify guards with "MS DI's sync scope Dispose throws for an IAsyncDisposable-only service" (TransformPipeline.cs:36, TransformPipelineAsync.cs:76, TransformLifetimeScope.cs:29, TransformLifetimeScopeAsync.cs:41, ServiceProviderLifetimeScope.cs:294). On net8+ that can't happen through DisposeScope, because the IAsyncDisposable branch is taken first — it's reachable only under the #if !NETSTANDARD2_0 exclusion, i.e. for netstandard2.0 consumers. The guards are still worth keeping (a user Dispose/Release can throw), but the stated reason is netstandard2.0-only. By the same token the first "deliberately out of scope" item — "IAsyncDisposable-only instances currently throw on the synchronous scope-disposal path" — is narrower than the write-up suggests; worth re-scoping that follow-up to netstandard2.0 before someone chases it on net9.

• The practical cost: because the async branch is unconditional, so is the pump offload. Every synchronous release on the Proactor — notably the HasPipeline probe release, on every outbound message via OutboxProducerMediator.MapMessageAsync — pays a Task.Run dispatch plus a blocking join even when the scope holds nothing async-disposable. That isn't the async-only edge case round 7 assessed it as. The deferred context-suppression variant removes it for the common all-synchronous case:

  var prev = SynchronizationContext.Current;
  SynchronizationContext.SetSynchronizationContext(null);
  try
  {
      var pending = asyncScope.DisposeAsync();
      if (pending.IsCompleted) { pending.GetAwaiter().GetResult(); return; }  // common case, zero hops
      pending.AsTask().GetAwaiter().GetResult();                             // safe: no context to post back to
  }
  finally { SynchronizationContext.SetSynchronizationContext(prev); }

  Same deadlock safety (no captured context for a user continuation to post to), no pool hop when there's nothing to await. Fine to keep deferred, but it applies to 100% of Proactor messages rather than a rare configuration, which I'd weigh differently.

4. Packaging: the new public surface rests on a transitive Microsoft.Bcl.AsyncInterfaces

netstandard2.0 is in BrighterTargetFrameworks (src/Directory.Build.props:43), and this PR puts ValueTask ReleaseAsync(...) on public interfaces and IAsyncDisposable on public TransformPipelineAsync<TRequest>. Neither Microsoft.Bcl.AsyncInterfaces nor System.Threading.Tasks.Extensions is referenced anywhere in the repo — they resolve transitively via System.Text.Json/System.Memory. It builds today, but a public contract resting on a transitively-supplied type is fragile: a later dependency bump that drops or downgrades it breaks netstandard2.0 consumers with a confusing missing-type error. Worth adding both explicitly to the existing '$(TargetFramework)' == 'netstandard2.0' ItemGroup in Paramore.Brighter.csproj.

---

Smaller notes

• HasPipeline (both builders): try { return mapper is not null; } finally { ... } is an unusual shape given the try body cannot throw. var m = Get<T>(); if (m is null) return false; Release(m); return true; reads more directly — though I appreciate the current form is robust to the body changing later.
• TransformLifetimeScope[Async].ReleaseTrackedObjects: _trackedObjects[0] + RemoveAt(0) over a List<T> is O(n²). Irrelevant at realistic transform counts; noting only in case these lists ever grow.
• ServiceProviderLifetimeScope.ReleaseAsync is async ValueTask even when CollectScopesToRelease returns empty (non-transient lifetime, untracked instance), so it allocates a state machine per no-op release. A non-async wrapper with if (list.Count == 0) return default; avoids it under Singleton/Scoped.
• PR description drift: the "deliberately out of scope" list still names ServiceProviderHandlerFactory.Release double-disposing transients, but that's fixed in this diff (ServiceProviderHandlerFactory.cs:100-123, direct disposal.Dispose() removed). Worth moving to the fixed set — the change is correct and the <remarks> explaining it is good.
• ControlBusMessageMapperFactory.cs still has no trailing newline.

What I checked and believe is right

• _transientScopes stack-per-instance, and CollectScopesToRelease's eager materialisation plus the ICollection.Remove(KeyValuePair) compare-and-remove with re-drain — the correct way to avoid removing a stack a concurrent GetTransient just pushed onto.
• GetOrCreateScoped's CompareExchange publish, the _disposed re-check with reclaim CAS, and Dispose's Interlocked.Exchange claim of _scope: exactly-once under both race directions, and the Lazy factory's ObjectDisposedException beats the null deref.
• The explicit void IAmAMessageMapperRegistryAsync.Release to dodge CS0121 for dual-interface mappers — right call, and the <remarks> tells future readers why not to "simplify" it back to public.
• Release-once via Interlocked.Exchange(ref _released, 1) shared across Dispose/DisposeAsync/finalizer, with the mapper released in a finally so a transform-scope throw can't orphan it.
• Reactor/Proactor casting the reflective object? handle to IDisposable/IAsyncDisposable — sound, the non-generic interfaces sit on the open base type.
• Test discipline is genuinely strong: each fix pinned by a test proven red against the specific reverted hunk, real BrighterAsyncContext pumps for the deadlock cases, forced races via Barrier rather than hoped-for interleavings, DisableParallelization with a pool floor for the pump tests. GatedScopeTracker's bounded SignalAndWait (fail rather than hang) is a nice touch.

Nothing here undoes the shape of the fix — items 1 and 2 are localised, 3 is additive. Item 1 is the one I'd want closed before merge, since it reintroduces this PR's own leak class on a common misconfiguration path.