Make Brighter logging instance-scoped (deprecate static ApplicationLogging)

[thomhurst]

Closes #4184

Problem

Logging was routed through the process-wide static ApplicationLogging.LoggerFactory, which the DI extensions populated from the container's ILoggerFactory. Because it is shared across the process, two or more Brighter instances (e.g. parallel test suites) would:

• log through the wrong service provider's ILoggerFactory (last initializer wins), and
• break each other on disposal — disposing one container disposed the ILoggerFactory the static still pointed at, causing ObjectDisposedException in other running instances.

Change

Logging is now instance-scoped: an ILoggerFactory flows as an instance through the object graph Brighter constructs, sourced from the container (or builder) that created the instance. Nothing writes to the static any more.

• CommandProcessorBuilder / DispatchBuilder gain ConfigureLogging(ILoggerFactory); the DI extensions resolve the container factory and pass it through (instead of assigning the static).
• Runtime types Brighter constructs — CommandProcessor, OutboxProducerMediator, Dispatcher, MessagePump, Reactor/Proactor, ConsumerFactory, the pipeline builders and the in-memory scheduler — take an optional trailing ILoggerFactory? loggerFactory = null; relational stores take an optional ILogger? logger = null.
• Transport and store packages: producer/consumer/channel factories thread the ILoggerFactory down to the leaf objects they construct, so it can be supplied for user-constructed pipelines too.
• s_logger static fields become instance _logger fields.
• ApplicationLogging is marked [Obsolete], defaults to NullLoggerFactory.Instance, and CreateLogger<T>() tolerates a disposed factory. It remains only as a crash-free, no-op fallback and will be removed in a future release.

All public constructor/builder changes are trailing optional parameters / additive methods, so existing call sites remain source-compatible.

Behaviour change

The static is no longer populated by the DI extensions. User-constructed transports/stores that are not passed an ILoggerFactory log to a no-op logger rather than to the (previously shared) static. The migration is described in the [Obsolete] message on ApplicationLogging; worth a note in the release notes.

Tests

• New InstanceScopedLoggingTests: a command processor logs only through its own container's factory (no cross-talk), is unaffected when a sibling container is disposed (no ObjectDisposedException), and falls back safely when no factory is registered.
• Updated the four MessageDispatch tests that assert on pump log output to pass a Serilog-backed test factory.
• Full solution builds clean (TreatWarningsAsErrors confirms no remaining references to the obsolete static); Paramore.Brighter.Core.Tests (752) and Paramore.Brighter.Extensions.Tests (102) pass.

[claude]

Code Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for this — a large but very disciplined change (176 files) that fixes a real, well-described problem: the process-wide ApplicationLogging.LoggerFactory causing cross-talk and ObjectDisposedException between Brighter instances. Execution is consistent and the migration doc is excellent. Feedback below, ordered by importance.

Strengths

• Consistent threading. The ILoggerFactory flows cleanly through the object graph (CommandProcessorBuilder/DispatchBuilder → CommandProcessor/OutboxProducerMediator/Dispatcher → ConsumerFactory → Reactor/Proactor/MessagePump → pipeline/transform builders), with a uniform ?? NullLoggerFactory.Instance fallback at every leaf. No NRE surface.
• Source compatibility preserved. All additions are trailing optional params / additive builder methods, so existing call sites still compile.
• Logger categories preserved. Nice detail keeping OutboxProducerMediator's logger categorized as CommandProcessor (matching the old s_logger), so existing log filters keep working.
• Crash-safe fallback. ApplicationLogging.CreateLogger<T>() tolerating a disposed factory is a sensible safety net for not-yet-migrated code.
• No stragglers. Confirmed ApplicationLogging is gone from all of src/ except its (obsolete) definition.

Main concern — migration doc understates the DI behaviour change

The doc says, for "Using the DI extensions (AddBrighter/AddConsumers)? No change required." That's true for CommandProcessor/Dispatcher/pipeline logging, but producers, consumers, outboxes and inboxes are still constructed by the user (e.g. new RmqProducerRegistryFactory(...).Create(), busConfiguration.Outbox) and handed to the configuration before the container exists. In BuildCommandProcessor/BuildOutBoxProducerMediator the container's ILoggerFactory is not injected back into those already-built objects. So even DI users will silently lose transport/store log output unless they thread the factory into those factories themselves.

This is arguably the most surprising regression for existing users (who today get transport logging "for free" via the static). Suggest the doc explicitly call this out under the DI section — not just under "constructing objects yourself" — since DI users do construct their producer registry / outbox manually in practice.

[Obsolete] + TreatWarningsAsErrors is a hard break for some consumers

src/Directory.Build.props sets TreatWarningsAsErrors=true, and the internal build is clean because nothing references the static anymore. But external consumers who reference ApplicationLogging and build with warnings-as-errors will get a build failure (CS0618) on upgrade, not a soft warning. Combined with the silent-logging behaviour change, this is breaking-change territory — please ensure it lands in a major version bump and is surfaced in release notes, not only in the migration guide.

Internal logging handlers in non-DI scenarios

RequestLoggingHandler/UseInboxHandler now take ILoggerFactory? = null. Under DI they get the factory injected when resolved from the container — good. But users who construct handlers via a hand-rolled IAmAHandlerFactory will now get null logging from these built-in handlers unless they thread the factory through. The migration doc covers transports/stores but not these built-in handlers; worth a sentence.

Test coverage — solid, with two gaps worth closing

• InstanceScopedLoggingTests are well-targeted and clearly motivated. However CommandProcessor_LogsThroughItsOwnContainersFactory_NotAnothers asserts only NotEmpty/Empty on captured entries — it leans on incidental PipelineBuilder log output from publishing with no subscribers. If that internal logging ever changes, the test silently weakens rather than failing meaningfully. Consider asserting on a specific known log line/category to pin the intent.
• The new tests exercise the DI path only. There's no direct test for the builder path (CommandProcessorBuilder.ConfigureLogging(...)/DispatchBuilder.ConfigureLogging(...)) or for OutboxProducerMediator instance-scoping. A small test that the explicit builder method routes logs to the supplied factory would lock in the public API contract.

Minor

• ApplicationLogging.LoggerFactory keeps a public mutable setter that is now a no-op w.r.t. Brighter. A user who sets it expecting log output will be silently disappointed; the XML doc notes this, but consider making the no-op nature even more prominent.
• InMemoryScheduler logs via _logger.LogError("...", state) (line ~301) rather than the source-generated Log.* partials used elsewhere — pre-existing, but inconsistent with the codebase's LoggerMessage convention if you're touching the file anyway.

Verdict

Architecturally the right fix, cleanly implemented, with genuinely good docs. The substantive asks: (1) sharpen the migration doc so DI users understand transport/store logging still needs explicit wiring, (2) treat the obsolete-under-TreatWarningsAsErrors + silent-logging combination as a major-version breaking change in release notes, and (3) add builder-path / outbox instance-scoping tests. None block the core mechanism, which looks correct.

🤖 Generated with Claude Code

[claude]

Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for tackling this — the underlying bug (a process-wide static ILoggerFactory causing cross-talk between parallel Brighter instances and ObjectDisposedException when one container is disposed) is real and the instance-scoping approach is the right fix. Overall this is a careful, mechanical change and it reads well. A few observations below.

Strengths

• Correct fix for a real problem. Flowing an ILoggerFactory instance through the object graph instead of a shared static eliminates both the last-writer-wins cross-talk and the use-after-dispose.
• Loggers are created eagerly in constructors (_logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<…>()). This is the key safety property — the leaf holds the ILogger, not the factory, so logging keeps working after the owning container is disposed. Good call.
• Backward-compatible surface for callers — trailing optional loggerFactory/logger params and additive ConfigureLogging(...) builder methods.
• Migration is complete. grep confirms no remaining ApplicationLogging.* references anywhere in src/, so TreatWarningsAsErrors against the [Obsolete] type genuinely guarantees the static is dead.
• Good, targeted tests. The three InstanceScopedLoggingTests cases (cross-talk isolation, sibling-dispose safety, null-factory fallback) map directly to the failure modes, and CapturingLoggerProvider throwing ObjectDisposedException on use-after-dispose is a nice way to make the regression observable rather than silent. The Initializer.TestLoggerFactory refactor is clean.

Issues / things to consider

1. Silent loss of logging for user-constructed stores/transports in DI setups (the main user-facing impact).
The DI extensions flow the container factory into CommandProcessor / OutboxProducerMediator / Dispatcher, but outboxes, inboxes and transports are typically user-constructed and passed into the builder (e.g. new MsSqlOutbox(config)), so they now default to NullLogger unless the user explicitly threads a logger/factory in. Previously these logged because the DI extension populated the static. This is acknowledged in the [Obsolete] message and PR body, but it's an easy-to-miss regression for existing DI users relying on outbox/inbox/transport logs. Worth giving prominence in the release notes (you flagged this), and a follow-up to have the relevant DI registrations inject the resolved factory into the stores they build would shrink the silent gap.

2. Framework handler subclasses don't forward the factory to their base RequestHandler<T>.
RequestLoggingHandler, UseInboxHandler, DeferMessageOnErrorHandler, RejectMessageOnErrorHandler each set their own _logger but call the base primary constructor with defaults, e.g.:

public DeferMessageOnErrorHandler(ILoggerFactory? loggerFactory = null)
{
    _logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<DeferMessageOnErrorHandler<TRequest>>();
}

The base RequestHandler<TRequest> (which logs PassingRequestFromTo / FallingBackFromTo) therefore stays null-backed even when a factory is supplied. Minor — only the base-class trace lines are affected — but forwarding (: base(loggerFactory: loggerFactory)) would make it consistent.

3. OutboxProducerMediator logs under the wrong category (pre-existing).
_logger = loggerFactory.CreateLogger<CommandProcessor>(); — the mediator creates its logger with the CommandProcessor category rather than its own type. This is carried over verbatim from the old ApplicationLogging.CreateLogger<CommandProcessor>(), so it's not introduced here, but since every line in this ctor is being touched it'd be a low-risk moment to correct it to the mediator's own type. (Respecting the repo's "Change Scope" guidance, leaving it is also defensible — your call.)

4. Adding ConfigureLogging to public builder interfaces is technically binary-breaking.
IAmACommandProcessorBuilder.ConfigureLogging and IAmADispatchBuilder.ConfigureLogging are new members on public interfaces. Source-compat is preserved for callers, but any external implementer of these interfaces would break. Almost certainly fine given these are framework builder contracts unlikely to be user-implemented — just noting it for the changelog/semver framing.

Performance & security

No concerns. The change is allocation-neutral in the hot path (loggers created once at construction; NullLoggerFactory.Instance/NullLogger<T>.Instance are singletons), and there are no security-relevant surfaces.

Test coverage

Solid for the regression itself. Two optional additions worth considering: (a) a test asserting the CommandProcessorBuilder.ConfigureLogging(...) non-DI path actually routes through the supplied factory, and (b) a case covering the "user-constructed outbox without a factory logs to no-op" behavior change, to pin the documented contract.

Nice work overall — the core mechanism is sound and the eager-creation detail is exactly right.

Reviewed by Claude — automated PR review.

[claude]

Review: Make Brighter logging instance-scoped (#4185)

Thanks for tackling this — routing logging through a process-wide static was a genuine latent bug, and the writeup of the two failure modes (last-writer-wins cross-talk + use-after-dispose on container disposal) is spot on. The overall approach is sound and the execution is consistent across a large surface (175 files). A few notes below, mostly minor.

What works well

• Right fix, right shape. Flowing an ILoggerFactory as an instance through the object graph is the correct model, and keeping every change additive (trailing optional ILoggerFactory? loggerFactory = null / ILogger? logger = null, additive ConfigureLogging builder methods) preserves source compatibility for existing call sites.
• Consistent null-fallback. The ?? NullLoggerFactory.Instance / ?? NullLogger<T>.Instance pattern is applied uniformly, so a missing factory degrades to a no-op rather than throwing. The NoLoggerFactoryRegistered_FallsBackSafely test pins this.
• Sensible DI division of labour. Types the host/ActivatorUtilities construct (e.g. TimedOutboxSweeper, the relational stores) take ILogger<T> so DI auto-injects, while manually-constructed types (OutboxArchiver, CommandProcessor, mediator) get the factory passed explicitly. That's the correct split.
• Obsolete shim is well done. ApplicationLogging defaulting to NullLoggerFactory.Instance and CreateLogger<T>() swallowing ObjectDisposedException makes the deprecated path crash-free, and the [Obsolete] message points users to all three migration routes.
• Tests target the actual bugs. InstanceScopedLoggingTests exercises exactly the cross-talk and dispose-a-sibling scenarios that motivated the change, and CapturingLoggerProvider throwing ObjectDisposedException after dispose is a nice way to surface a regression if a factory reference ever leaks again.

Observations / questions

1. Message-creator leaves silently lost their logging path (main substantive item). KafkaMessageCreator, RedisMessageCreator, and both RmqMessageCreators still hold a hardcoded static logger, now NullLoggerFactory.Instance.CreateLogger<...>() instead of ApplicationLogging.CreateLogger<...>(). Previously, in a DI setup, these routed through the container's factory; now their diagnostics (malformed-header warnings, correlation-id/handled-count/delay parse failures, FailedToReadTheValueOfHeader, etc.) are dropped even when the user has configured logging. The owning consumer already has a loggerFactory in scope (e.g. RmqMessageConsumer), so this looks threadable. Suggest either threading the factory down (making the creator instance-scoped where it's static-called) or explicitly calling out in the PR description that these creator diagnostics become no-ops — right now this is a quiet observability regression that the "user-constructed transports/stores" caveat doesn't quite cover, since these are Brighter-constructed.

2. Behaviour change is runtime-silent, not just compile-time. Code reading ApplicationLogging.LoggerFactory (yours or third-party) still compiles but now silently observes a no-op factory rather than the container's. The [Obsolete] attribute only nudges at compile time. Please make sure the release notes call this out prominently as a behavioural break, not just a deprecation — the PR body flags it as "worth a note," which I'd upgrade to "must."

3. Test coverage skews to the CommandProcessor/DI path. The new instance-scoping tests only cover IAmACommandProcessor. The Dispatcher/MessagePump/Reactor/Proactor threading is exercised only indirectly via the four updated MessageDispatch tests. A small test asserting two Dispatchers don't cross-talk would lock in the consumer side too, given that's half the object graph touched here. Optional given the scope.

4. Minor consistency nit. Relational stores take ILogger? while everything else takes ILoggerFactory?. The rationale (DI injects ILogger<T> directly for these) is sound, but the asymmetry is non-obvious — a one-line comment at those constructors explaining "factory is injected as ILogger<T> by DI here" would help future maintainers.

5. InstanceScopedLoggingTests assertions lean on an implementation detail. Both passing tests assert the other container's capture is Empty, which holds only because resolving an IAmACommandProcessor doesn't itself log. That's true today and the tests are fine, but if construction ever starts logging these become confusingly red. A comment noting "resolution must not log" would document the assumption.

Verification

• Confirmed no remaining ApplicationLogging or live s_logger references in src/ outside the obsolete class and the four message creators (which are intentionally retained as no-ops).
• [Obsolete] usage inside the DI extension is correctly pragma-suppressed; with TreatWarningsAsErrors a clean build implies no stray references to the obsolete static — consistent with the PR's claim.

Nice, careful piece of work overall. The message-creator gap (#1) is the only thing I'd want resolved or explicitly documented before merge; the rest are polish.

🤖 Generated with Claude Code

[iancooper]

Alright, we will need to look over this one carefully, as it was previously a choice to avoid passing loggers throughout the application. It may not be a breaking change, but it may change behavior for V10 users who don't transition, potentially resulting in a silent failure. That would mean, IMO, it would be V11, and it would be preferable to force success rather than allow silent failure. In addition, it may change log messages.

Not necessarily adverse, but didn't really want to trigger V11 work until SEP/OCT

[thomhurst]

Agreed it's a breaking change for anyone manually constructing these classes. We can hold off until V11 if you were happy with this change and park for now if needed.

For MEDI users I don't believe they'd have to tweak any code.

But also happy to discuss the design further. The current behaviour is a problem though for any two or more brighter's running under the same process. We noticed it earlier in a test suite.

What do you mean by force success btw? Remove the NullLogger fallback?

[claude]

Review: Make Brighter logging instance-scoped (#4185)

Thanks for tackling this — the use-after-dispose and cross-talk caused by the process-wide static were real, nasty bugs (especially under parallel test suites), and the instance-scoped approach is the right fix. The refactor is large but overwhelmingly mechanical and consistent: s_logger → _logger, an optional trailing ILoggerFactory? loggerFactory = null threaded through the object graph, and a uniform ?? NullLoggerFactory.Instance fallback that keeps everything crash-free. I traced the DI → builder → CommandProcessor/Dispatcher → ConsumerFactory → Reactor/Proactor/MessagePump → PipelineBuilder/TransformPipelineBuilder paths and the wiring is correct end-to-end. [Obsolete]-ing ApplicationLogging rather than deleting it, plus the TreatWarningsAsErrors gate to prove no internal references remain, is a clean migration.

A few things worth addressing or at least calling out:

1. Message creators silently lose logging entirely (behavioural regression, not in the PR description)

RmqMessageCreator (sync + async), KafkaMessageCreator, and RedisMessageCreator were changed from ApplicationLogging.CreateLogger<T>() to a hardwired NullLogger<T>.Instance:

private static readonly ILogger s_logger = NullLogger<RmqMessageCreator>.Instance;

Under DI, the old static was populated with the container's real factory, so these creators previously logged through it. Now they log to a no-op for everyone, with no way to ever supply a real logger. The messages being silenced are exactly the diagnostics an operator wants — ExpectedHeaderError, ExpectedBodyError, FailedToCreateMessageFromKafkaOffset, etc. — i.e. "we received a malformed/unparseable message."

• KafkaMessageCreator.CreateMessage is an instance method, and KafkaMessageConsumer does _creator = new KafkaMessageCreator(); at KafkaMessageConsumer.cs:270 while already holding a loggerFactory. Threading the factory through here is trivial and would preserve the diagnostics — this one looks like a straightforward miss.
• RmqMessageCreator / RedisMessageCreator use static CreateMessage methods, so it's more awkward — but an ILogger parameter on the static method (passed from the calling consumer) would keep them alive.

If silencing these is intentional, it should at least be noted in the PR's "Behaviour change" section; right now that section only mentions user-constructed transports/stores falling back to no-op, not these always-on-no-op leaf objects.

2. Interface addition is a breaking change for implementers (minor)

ConfigureLogging(ILoggerFactory) is added to the public IAmACommandProcessorBuilder interface (and the DispatchBuilder equivalent). The PR says "all public constructor/builder changes are trailing optional parameters / additive methods, so existing call sites remain source-compatible" — true for callers, but adding a member to a public interface is a source/binary break for anyone who implements it. Unlikely anyone implements Brighter's builder interfaces, but the claim is slightly overstated; worth a one-line caveat in the release notes.

3. Per-message CreateLogger on the hot path (minor perf)

s_logger was previously created once per type. Now PipelineBuilder<T> is constructed per Send/Publish and calls loggerFactory.CreateLogger<PipelineBuilder<T>>() each time, and RequestHandler<T>'s primary constructor calls CreateLogger per handler instantiation (i.e. per message). The default LoggerFactory.CreateLogger caches by category but takes a lock to do so, so this adds a small amount of locked dictionary work per message/per handler on the dispatch hot path. Almost certainly negligible, but flagging since the previous static avoided it entirely. Not a blocker.

Test coverage

InstanceScopedLoggingTests is well-targeted — it directly exercises the three behaviours that motivated the change (no cross-talk, survives sibling-container disposal, safe fallback with no factory registered), and the CapturingLoggerProvider throwing ObjectDisposedException post-dispose is a nice way to actually surface a use-after-dispose rather than just asserting on output. The Initializer.TestLoggerFactory migration for the four MessageDispatch tests is clean.

Gap: the new tests only cover the CommandProcessor/DI path. There's no coverage for the Dispatcher/ServiceActivator instance-scoped path or for transport leaf objects receiving the factory — which is relevant to finding #1 above.

Summary

Solid, well-executed fix for a genuine concurrency/disposal bug. The main thing I'd want addressed before merge is #1 — at minimum thread the factory into KafkaMessageCreator, and document the message-creator logging loss in the behaviour-change notes. The rest are minor / release-note items.

[codescene-delta-analysis]

Code Health Improved (3 files improve in Code Health)

Gates Failed
Prevent hotspot decline (2 hotspots with Complex Method)
Enforce advisory code health rules (4 files with Complex Method, Constructor Over-Injection, Code Duplication)

Our agent can fix these. Install it.

Gates Passed
2 Quality Gates Passed

Reason for failure

Prevent hotspot decline	Violations	Code Health Impact
OutboxProducerMediator.cs	1 rule in this hotspot	6.97 → 6.95	Suppress
KafkaMessageConsumer.cs	1 rule in this hotspot	6.87 → 6.86	Suppress

Enforce advisory code health rules	Violations	Code Health Impact
PipelineBuilder.cs	1 advisory rule	7.79 → 7.33	Suppress
MessagePump.cs	1 advisory rule	10.00 → 9.69	Suppress
OutboxProducerMediator.cs	1 advisory rule	6.97 → 6.95	Suppress
KafkaMessageConsumer.cs	1 advisory rule	6.87 → 6.86	Suppress

See analysis details in CodeScene

View Improvements

File	Code Health Impact	Categories Improved
SqsInlineMessageCreator.cs	8.96 → 9.54	Overall Code Complexity
S3LuggageStore.cs	8.60 → 8.88	Primitive Obsession, String Heavy Function Arguments
S3LuggageStore.cs	8.60 → 8.88	Primitive Obsession, String Heavy Function Arguments

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for tackling this — moving off the process-wide static is the right call, and issue #4184 is a real correctness bug (cross-talk + use-after-dispose between Brighter instances). The approach is clean: trailing optional ILoggerFactory?/ILogger? parameters and additive ConfigureLogging(...) builder methods keep every public call site source-compatible, and the DI extensions now flow the container factory through as an instance instead of writing the static. I verified there are no remaining references to ApplicationLogging or s_logger anywhere in src/, so the migration is complete and consistent.

A few observations, mostly minor, plus one I would like you to confirm is intentional.

1. Base RequestHandler relay logs are now silent in all paths (please confirm intent)

RequestHandler<TRequest> / RequestHandlerAsync<TRequest> gained a primary-constructor ILoggerFactory? loggerFactory = null, and _logger falls back to NullLoggerFactory.Instance when it is null (src/Paramore.Brighter/RequestHandler.cs:52, RequestHandlerAsync.cs). The problem: nothing forwards a factory to that base constructor. I grepped — no built-in handler calls : base(loggerFactory: ...), and the MS DI container cannot satisfy a base-class primary-constructor parameter (it binds the derived constructor). The built-in handlers that take a logger (RequestLoggingHandler, UseInboxHandler, etc.) inject their own ILogger<T> for their own log lines, but when they call base.Handle(...) the base-class relay lines — Log.PassingRequestFromTo / Log.FallingBackFromTo (RequestHandler.cs:113,148) and the async equivalents — go to NullLogger. Pre-change, those lines logged through the DI-populated static, so in the DI happy path they were visible. After this PR they are silent everywhere, including DI. That is an observability regression not covered by the user-constructed transports/stores note in the PR description. If it is a deliberate trade-off (these are verbose debug lines) it would be worth saying so in the migration notes; otherwise the base needs a way to receive the factory (e.g. PipelineBuilder setting it on handlers it chains, since PipelineBuilder already holds a logger).

2. OutboxProducerMediator logger category

_logger = loggerFactory.CreateLogger<CommandProcessor>() (src/Paramore.Brighter/OutboxProducerMediator.cs:121) — categorised as CommandProcessor, not OutboxProducerMediator. This faithfully preserves the old static category, so it is not a regression, but since you are already touching every line it would be a good moment to fix the mislabelled category (or leave a comment that it is deliberate for log-filter back-compat).

3. Activator.CreateInstance positional binding (not a bug, just fragile)

BuildOutBoxProducerMediator passes loggerFactory as the trailing positional arg to Activator.CreateInstance (ServiceCollectionExtensions.cs:754). This is correct today because loggerFactory is the last constructor parameter — but it is reflection-based, so an arg-order mismatch would compile fine and throw only at runtime. Worth a one-line comment tying the positional order to the constructor signature so a future reorder does not silently break it.

Test coverage

The new InstanceScopedLoggingTests + CapturingLoggerProvider are nicely targeted — the dispose-A-does-not-break-B and no-factory-registered-falls-back-safely cases directly pin the #4184 bug, and the capturing provider throwing ObjectDisposedException on use-after-dispose is a clean way to surface the original failure mode. Gaps worth closing: (a) all three tests publish an event with no subscribers, so they only exercise the PipelineBuilder log lines via CommandProcessor._logger — they never run an actual handler pipeline, so they do not prove handlers resolved via DI log through the right factory, and would not catch the base-handler regression in point 1; consider a test that registers a real handler and asserts its log output lands in the right capture. (b) No test covers the ConfigureLogging(...) builder path directly (only the DI path) — a quick CommandProcessorBuilder.ConfigureLogging(factory) assertion would lock in the non-DI flow.

Minor

ApplicationLogging.CreateLogger<T>() swallowing ObjectDisposedException to return NullLogger<T> (ApplicationLogging.cs:41) is a sensible crash-free fallback for stragglers; good defensive touch given the type is obsolete-but-present.

Overall this is a solid, well-scoped refactor that fixes a genuine bug with good backward-compat discipline. The main thing I would want resolved before merge is confirming whether the base-handler relay logging going silent (point 1) is intended, and ideally a test that exercises a real handler pipeline.

Automated review — verified against the merge of this branch into master; I was unable to run the build/tests in this environment, so the compile-clean claim is taken on trust.

[iancooper]

What do you mean by force success btw? Remove the NullLogger fallback?

Yes, because I get that stops users having compilation issues, but it feels like it could silently fail on folks who where not expecting it.

[iancooper]

But also happy to discuss the design further. The current behaviour is a problem though for any two or more brighter's running under the same process. We noticed it earlier in a test suite.

I think it may help with some other aspects like testing too. We currently have to use a test capture context and I am not sure that is the best route