[iancooper]

Closes #377. Also closes #378, #379 and #380.

What this does

A Darker handler can now share the Scoped services of an ASP.NET Core request, such as an EF Core DbContext, with the controller and Brighter handlers. Register Scoped handlers and call AddDarkerRequestScope() from the new Paramore.Darker.Extensions.AspNetCore package. Making the QueryProcessor Scoped never achieved this, and the guidance in CLAUDE.md and the README is corrected.

Spec: specs/015-di_lifetime_conformance/. Design: ADRs 0022, 0023 and 0024. ADR 0014 stays Accepted and is noted as partially superseded.

• Make pipeline teardown robust: release every item, never throw, dispose scope last #378, teardown (ADR 0022): each release is isolated and logged at Error, the scope is disposed last, and teardown never throws or hides the handler's own exception. The async and stream paths await DisposeAsync and release through the factory that created the handler. A build that fails part-way releases what it already created. A throwing logging provider cannot break teardown.
• Replace factory-held scope table with a pipeline scope handle (CreatePipelineScope / IAmALifetime.PipelineScope) #379, pipeline scope handle (ADR 0023): IAmAScope, CreatePipelineScope() on the handler factories, and IAmALifetime.PipelineScope in place of Add.
• Adopt ambient request scope: ScopeAffinity option and Paramore.Darker.Extensions.AspNetCore package #380, ambient scope adoption (ADR 0024): ScopeAffinity, IAmAScopeProvider, IAmAServiceProviderScope, DarkerOptions.DefaultScopeAffinity (default AlwaysNew, so existing behaviour is unchanged), AddDarkerScopeProvider<T>(), and the new Paramore.Darker.Extensions.AspNetCore package. Adoption requires HandlerLifetime = Scoped. An offered scope that is the root provider, from another container, or faulty is declined with one Warning. Darker never disposes an adopted scope.

Lifetime validation stays in #305.

Breaking changes

Custom factory implementations must change. The full list, with migration notes, is in the spec 015 section of release_notes.md:

• async factory Release becomes ValueTask ReleaseAsync, and IAmALifetime extends IAsyncDisposable;
• handler factories gain CreatePipelineScope();
• IAmALifetime.Add is removed;
• decorators Darker registers are Transient unless HandlerLifetime is Singleton.

Under JoinAmbient, queries in one scope share its Scoped services, including a Scoped handler instance. Concurrent queries in one request need AlwaysNew.

Samples

SampleMinimalApi gains a /request-scope endpoint that shows the endpoint and a Scoped handler seeing the same Scoped service.

Verification

• dotnet build Darker.Filter.slnf -c Release succeeds, and the full suite passes on net8.0 and net9.0.
• Two adversarial code review passes were run. Findings were fixed, apart from the ones below.

Not in this PR

• Removing the two existing InternalsVisibleTo entries, and a test that finds an internal type by name: Remove InternalsVisibleTo from the test setup #382.
• xUnit1051 warnings in test files that predate this branch.
• The optional Singleton-path benchmark (T5.9) was not run.
• Sample files carry no licence header, matching the existing samples.

Review notes

This is a large diff (about 190 commits). The Phase 3 and Phase 4 tests were reviewed in batches, and the Phase 4 ralph run (T4.6 to T4.9) was reviewed afterwards. T4.8's second mutation is covered by the disposed-scope probe, which was accepted.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MiLKqEzdGHZdixwJUcenrj

[iancooper]

@claude Let's review this, as it is a significant change. We have two goals here: improve our handling of the lifetime of components that Brighter creates through a factory, to ensure that we don't leak, and we don't carry state between operations if the user did not ask for that. Second, to allow us to share a ServiceCollection and resolution, with an HTTPContext (and Brighter) such that we would resolve a dependency with state, such as an EF DbContext, from the same ServiceCollection scope

[claude]

Claude encountered an error after 3m 17s —— View job

---

Review: DI lifetime conformance with Brighter (#377)

Todo list

• Gather context (PR description, ADRs 0022/0023/0024, changed files)
• Read core teardown changes (PipelineBuilder.cs, QueryLifetimeScope.cs, IAmALifetime.cs)
• Read scope-handle changes (IAmAScope.cs, IQueryHandlerFactory(Async).cs, SimpleHandlerFactory.cs)
• Read ambient scope adoption changes (ScopeAffinity.cs, IAmAScopeProvider.cs, ServiceProviderComponentFactory.cs, BorrowedServiceProviderScope.cs)
• Read the AspNetCore extension package (HttpContextScopeProvider.cs, HttpRequestScope.cs, DI registration)
• Cross-check against ADRs 0022/0023/0024 and the doc/README/CLAUDE.md corrections
• Post review findings

Goal 1 — lifetime/leak handling (ADR 0022 + 0023)

This is a solid, well-contained design:

• PipelineBuilder<TResult> (src/Paramore.Darker/PipelineBuilder.cs) now creates the per-query QueryLifetimeScope before resolving the handler (PipelineBuilder.cs:69, :131, :291), so a scope created during resolution is tracked even if a later step in Build/BuildAsync throws. Decorators are added to their tracking list the moment they're created (:272, :433), with a comment explaining this is so Dispose can release them even if InitializeFromAttributeParams throws — good defensive sequencing.
• Teardown never throws: every release (TryRelease/TryReleaseAsync) is wrapped and logged at Error (PipelineBuilder.cs:534-556), and even the logging calls themselves are wrapped (TryLogTeardownFault, :572-582) so a faulty logging provider can't break teardown — matches ADR 0022's guarantee.
• The scope is always disposed last (Dispose()/DisposeAsync() at :447-468), and release goes through the factory that actually created the handler (_handlerCreatedByAsyncFactory, :487-500) rather than assuming sync vs async — this correctly avoids releasing through the wrong factory.
• Checked for a sync/async mismatch leak: QueryProcessor.Execute (sync) only ever calls Build(), which populates only _decorators, so sync Dispose() not touching _asyncDecorators/_streamDecorators is safe — those lists stay null and are only populated (and only disposed) on the async/stream paths, which always go through DisposeAsync() (QueryProcessor.cs:107, :169). No leak here.
• CreatePipelineScope() on ServiceProviderComponentFactory (src/Paramore.Darker.Extensions.DependencyInjection/ServiceProviderComponentFactory.cs:62-75) returns null for Singleton (nothing to dispose) and otherwise either adopts an ambient scope (never disposed, per BorrowedServiceProviderScope) or creates a ServiceProviderLifetimeScope that Darker owns and disposes exactly once. If CreatePipelineScope() itself throws (e.g. AmbientScopeSourceException from a faulty provider), nothing has been allocated yet, so there's nothing to leak, and _lifetime stays null-safe through PipelineBuilder.Dispose/DisposeAsync.

I didn't find a leak or double-dispose path in this part of the diff.

Goal 2 — sharing scope with HttpContext/EF DbContext (ADR 0024)

• ScopeAffinity.JoinAmbient + HandlerLifetime = Scoped is the only combination that adopts; everything else (wrong lifetime, no provider, non-IAmAServiceProviderScope offering, root container, offering from another container, a throwing provider/Services getter) is declined with a single Warning per reason via AdoptionDiagnostics (ServiceProviderComponentFactory.cs:79-159), never an exception that would break the query. This matches ADR 0024's decision ladder.
• The "is this the root container" check (IsRoot, ServiceProviderComponentFactory.cs:144-145) relies on the Microsoft.Extensions.DependencyInjection implementation detail that only the root provider answers IServiceProvider and IServiceScopeFactory with the same object — subtle, but it's exercised by When_the_offered_scope_is_not_an_IAmAServiceProviderScope_should_use_owned_scopes_and_log_one_warning.cs and the AspNetCore test suite (test/Paramore.Darker.Extensions.AspNetCore.Tests/), which covers no-HttpContext, throwing providers, call-order independence, and successive requests reusing separate scopes. Good edge-case coverage for something this implementation-detail-dependent.
• HttpContextScopeProvider/HttpRequestScope (src/Paramore.Darker.Extensions.AspNetCore/) are a clean, minimal non-owning handle: Dispose()/DisposeAsync() are no-ops because ASP.NET Core owns the request scope, consistent with "Darker never disposes an adopted scope."
• Docs are corrected consistently: CLAUDE.md, README.md, and the new SampleMinimalApi /request-scope endpoint all now say the same thing — a Scoped QueryProcessor does not share the request's DbContext; what's needed is HandlerLifetime = Scoped + AddDarkerRequestScope(). The old (incorrect) "QueryProcessor must be Scoped" guidance is gone everywhere I checked (git diff origin/master...HEAD -- CLAUDE.md README.md).
• The documented hazard — concurrent queries in one request sharing a Scoped handler instance and tripping EF Core's "no concurrent operations" guard under Task.WhenAll — is called out in the XML docs on ScopeAffinity.JoinAmbient, AddDarkerScopeProvider, and AddDarkerRequestScope, and in CLAUDE.md/README. That's the right way to handle an inherent sharp edge of the feature rather than trying to paper over it.

Minor observations (not blocking)

• samples/SampleMinimalApi/Program.cs still has no trailing newline at EOF (pre-existing, not introduced by this PR).
• I was not able to run dotnet build/dotnet test in this sandbox (the command required interactive approval I couldn't grant), so I relied on static review plus the test files already present under test/Paramore.Darker.Extensions.AspNetCore.Tests/ and test/Paramore.Darker.Extensions.Tests/. The PR description states the full suite passes on net8.0/net9.0 and that two adversarial review passes already ran — if you want, a maintainer/CI run should confirm that holds for the final state of the branch.

Summary

Both stated goals are met by the design and the code backing it: teardown is now isolated, logged, scope-last, and never-throwing (#378/#379), and AddDarkerRequestScope() + HandlerLifetime = Scoped gives Darker handlers genuine access to the ASP.NET Core request's Scoped services, including an EF Core DbContext (#380). The decline-with-warning behavior around ambient scope adoption is conservative and well-tested. I found no correctness issues in the teardown or scope-adoption logic; the main residual risk is the inherent one the PR itself documents (concurrent queries sharing a Scoped handler under JoinAmbient), which is a design tradeoff, not a bug.