Make DI handler/decorator factories lifetime-aware (#329)#330
Merged
Conversation
Add the approved requirements and Accepted ADR 0014 for making Darker's DI handler/decorator factories lifetime-aware (Singleton/Scoped/Transient), matching Brighter, so handlers, decorators, and their scoped dependencies (e.g. EF Core DbContext) are created and released per their configured lifetime. Includes the adversarial review records for both phases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rationale (#329) Adds the approved tasks.md (Tidy-First structural-then-TDD breakdown) and the adversarial tasks review record. Corrects two issues found during task review: - requirements FR4/NFR2/AC9: the default Transient path's per-query disposal of injected disposables is a deliberate improvement, not 'identical to current' (the current factory disposes only the handler object, not its injected deps). - ADR 0014 Decision 4 §2: retract the false 'shared Singleton could be constructed twice' claim (a container Singleton is one-per-container regardless of factory caches); the merge is justified structurally, and is sequenced into the structural implementation step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Structural change (Tidy First, task S1): introduces the per-query lifetime role IAmALifetime (public) and its default internal implementation QueryLifetimeScope, which tracks per-query disposables and disposes them in reverse order exactly once. No call sites yet, so behaviour is unchanged and the suite stays green. ADR 0014 Decision 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Structural change (Tidy First, tasks S2-S4): widen the four public factory interfaces (IQueryHandlerFactory/...Async, IQueryHandlerDecoratorFactory/...Async) so Create and Release take an IAmALifetime, and thread a per-query lifetime through PipelineBuilder. - S2: add the IAmALifetime parameter to Create/Release on all four interfaces. - S3: update every implementor to the new signatures, ignoring the token for now (SimpleHandlerFactory, SimpleHandlerDecoratorFactory, FactoryFuncWrapper, the two ServiceProvider DI factories, and the RecordingHandler/Decorator test doubles). - S4: PipelineBuilder creates one QueryLifetimeScope at the start of Build/BuildAsync (before any Create), threads it through every Create/Release, and disposes it last in Dispose() (null-safe). The lifetime owns no scope yet, so behaviour is unchanged. ADR 0014 Decisions 2 and 5. Suite stays green (76 + 8 tests on net8.0 and net9.0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
S5 (structural, Tidy First). Merge ServiceProviderHandlerFactory and ServiceProviderHandlerDecoratorFactory into a single internal ServiceProviderComponentFactory implementing all four factory interfaces. The ctor now takes (IServiceProvider, ServiceLifetime handlerLifetime) per ADR Decision 6, but the lifetime is not yet used — behaviour is identical to the previous naive factories (resolve from provider, dispose any IDisposable on release). BuildQueryProcessor constructs one instance and threads it into all four HandlerConfiguration slots so handler and decorator share one cache/scope once Phase 2 makes it lifetime-aware (ADR Decision 4). Suite stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
B0 (test infrastructure, no production code). Adds the observability probe used by the Phase 2 lifetime acceptance tests: - ITrackedDependency / TrackedDependency + thread-safe DependencyTracker (Interlocked construction counter; IsDisposed flag). - TrackedQuery + sync/async handlers that return the injected dependency. - DecoratedTrackedQuery + sync/async handlers and TrackedDecorator(Async) + attributes, where handler and decorator both receive ITrackedDependency and record it into the result (for cross-role scoped-sharing assertions). - TrackedDependencyScenario builder wiring a real MS DI container with AddDarker, the tracked handlers/decorators, and optional ValidateScopes. No production code and no [Fact] yet; the behavioural tests arrive per-AC under /test-first in B1+. Suite stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#329) B1 (AC3 / FR1, FR2). The DI factory now resolves non-Singleton components from a per-query child IServiceScope (new internal ServiceProviderLifetimeScope), created from the captured provider's IServiceScopeFactory and attached to the per-query IAmALifetime via Add(scope). The lifetime disposes the scope when the pipeline completes, so an injected Transient disposable dependency is now torn down deterministically per query instead of leaking until the root provider is disposed. Release becomes a no-op: scoped/transient teardown is owned by the scope, and Singletons are never disposed by Darker. Test (sync + async): with HandlerLifetime = Transient, executing a query twice constructs a fresh dependency each time and disposes it after each pipeline. Cross-Create scope sharing (handler + decorator) is intentionally not yet implemented; B3 drives it red->green. The B0 scenario helper now registers a NullLoggerFactory so ApplicationLogging can initialise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
B2 / AC1+AC2 (FR3, FR2): with HandlerLifetime = Singleton, two queries reuse one injected dependency (ConstructionCount == 1, ReferenceEquals), the second query succeeds with no ObjectDisposedException, and the dependency is never disposed by Darker (IsDisposed == false after the pipeline). Covers both Execute and ExecuteAsync. This is an acceptance lock with no production change. Defect 1 (singleton disposed after one query) was already neutralised structurally in S5 (a4ec2d6): ServiceProviderComponentFactory.Release is a no-op and a Singleton handler is container-managed, so GetService returns the same instance and nothing disposes it. The ADR-mandated singleton cache (Decision 3) yields no observable behaviour for a container-managed singleton (ADR Decision 4 §2), so per the mandatory TDD rule and no-scope-creep it was not added; the test stands as the AC1/AC2 lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) B3 / AC4 (FR4): with HandlerLifetime = Scoped, a Scoped dependency injected into both the handler and its decorator in one execution now resolves to the SAME instance (ConstructionCount == 1, ReferenceEquals) and is disposed after the pipeline. Covers Execute and ExecuteAsync. ServiceProviderComponentFactory previously created a new child scope on every Create call, so handler and decorator received two distinct scoped instances. It now get-or-creates one ServiceProviderLifetimeScope per execution, keyed on the IAmALifetime token via a ConditionalWeakTable (weak keys, no per-query mutable field on the shared factory). The scope is attached to the token via Add and disposed once by PipelineBuilder.Dispose(). The Singleton branch is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…329) B4 / AC5 (FR5), defect 2: with the default Singleton QueryProcessor, a Scoped dependency (an EF Core DbContext stand-in) injected into the handler resolves from the per-query child scope and is disposed after the pipeline, with ValidateScopes = true so resolving Scoped from the root provider would throw. Covers Execute and ExecuteAsync. Acceptance lock, no production change: the IServiceScopeFactory-rooted child scope added in B1 already fixes defect 2. Verified the lock has teeth — temporarily resolving the non-Singleton branch from the captured root provider makes both variants fail with "Cannot resolve scoped service ... from root provider"; the child-scope resolution passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…329) B5 / AC6 (FR6, NFR3): two queries run concurrently through one shared Singleton processor, each parked mid-pipeline on a barrier so both are provably in flight. They resolve distinct scoped dependencies (isolated scopes), and completing pipeline A disposes A's scope while B's stays alive. Covers Execute and ExecuteAsync. Acceptance lock, no production change: each execution owns its own IAmALifetime (one per Build/BuildAsync) and thus its own child scope via the ConditionalWeakTable, so concurrent pipelines are already isolated. Adds a barrier-aware ConcurrentTrackedQuery test double and registers it in TrackedDependencyScenario. Verified the lock has teeth — resolving the non-Singleton branch from the captured root provider makes both variants fail (shared root-scoped dependency); child-scope resolution passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
B6 / AC7 (FR9): when the handler throws or the async pipeline is cancelled, the per-query dependency is still disposed — disposal is owned by PipelineBuilder.Dispose() running in QueryProcessor's using finally. Theory over Transient and Scoped; covers throw (sync + async) and cancellation (async). Acceptance lock, no production change: the failure-path disposal is already provided by S4 (lifetime created first, disposed in the finally). Adds ThrowingTrackedQuery and CancellingTrackedQuery doubles. Verified the lock has teeth — master-like root resolution leaves the dependency undisposed on the failure path (all 6 cases fail); child-scope resolution passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…329) B7 / AC8 (FR7, FR8). Asserts the decorator's injected dependency follows the same Singleton/Transient rules as a handler's, via DecoratedTrackedQuery: - Singleton: constructed once, reused across queries, never disposed. - Transient: fresh per query, disposed after the pipeline. Green acceptance lock — the merged ServiceProviderComponentFactory already applies identical logic to the decorator Create/Release paths (B1 child-scope disposal + S5 no-op Release), so no production change is needed. Teeth verified on the Transient case via master-like root resolution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
B8 / AC9 (NFR2). With no HandlerLifetime configured (default Transient) and the default Singleton QueryProcessor, the injected dependency is constructed fresh per query and disposed after each pipeline. Adds TrackedDependencyScenario.BuildWithDefaultLifetime, which calls AddDarker() without setting HandlerLifetime so the test exercises the default value rather than an explicit setting (extracts a shared BuildCore). Green acceptance lock — the default path routes through B1's per-query child-scope disposal, so no production change is needed. Teeth verified via master-like root resolution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verification task, no new behaviour. Every B1-B8 behavioural test pairs an Execute (void) variant with an ExecuteAsync (async Task) variant; B6's failure path covers throw-sync, throw-async and cancel-async (sync cancellation is not applicable). No parity gaps found. Fills the AC1-AC10 -> test-file mapping and removes a stray editor artifact from the end of tasks.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds acceptance-verification.md mapping every AC to its passing test file plus
the NFR4 coverage matrix (3 lifetimes x {handler, decorator} x {sync, async}).
Marks F1 (build+test green on net8/net9, 212 passing), F2 (coverage audit) and
F3 (acceptance record) complete.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a team-committed .claude/settings.json that allowlists read-only shell commands (file inspection, system info, read-only git/dotnet) so they no longer trigger permission prompts. Mutating commands (sed -i, bare git, etc.) are deliberately excluded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
4 Quality Gates Passed
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes Darker's DI-backed handler and decorator factories lifetime-aware (Singleton / Scoped / Transient), matching Brighter, so handlers, decorators, and their injected dependencies (e.g. an EF Core
DbContext) are created and released per their configured lifetime.Fixes #329. Per ADR
0014-factory-component-lifetime(Accepted). Targeted at V5 — this is a breaking change to the public factory interfaces.Defects fixed
Releasedisposed anyIDisposable, so the next query received a disposed singleton →ObjectDisposedException. Darker'sReleaseis now a no-op; container-managed singletons are never disposed by Darker (AC1/AC2).QueryProcessor, the factory captured the root provider, so a ScopedDbContextnever bound to the request. Each query now owns a childIServiceScope(viaIServiceScopeFactory), so scoped deps resolve from the per-query scope even under a Singleton processor — verified withValidateScopes = true(AC5).Design
ServiceProviderComponentFactory. A handler and its decorators share one per-query child scope, keyed on the per-query lifetime token via aConditionalWeakTable(no per-query mutable state on the shared singleton factory).IAmALifetimetoken and disposed exactly once inPipelineBuilder.Dispose(), invoked fromQueryProcessor'susingfinally — so it is released even when the handler throws or the async pipeline is cancelled (AC7).HandlerLifetime(Transient) andQueryProcessorLifetime(Singleton) are unchanged; no generalIDarkerOptionsservice (BuildQueryProcessorpassesoptions.HandlerLifetimestraight into the factory).Breaking change (public interfaces)
The factory abstractions gain an
IAmALifetimeparameter:(same shape for
IQueryHandlerFactoryAsync,IQueryHandlerDecoratorFactory,IQueryHandlerDecoratorFactoryAsync). New publicIAmALifetimerole +QueryLifetimeScope.Testing
dotnet build Darker.Filter.slnf -c Release→ 0 errors.dotnet test Darker.Filter.slnf -c Release --no-build→ 212 passing (Core 76 + Extensions 30, each on net8.0 and net9.0), 0 failed, 0 skipped.specs/008-factory_component_lifetime/acceptance-verification.md.Process
Built spec-first (Requirements → ADR 0014 → tasks → TDD implementation). Structural changes (Tidy First) landed separately from behavioural ones; each commit is purely structural, behavioural, or docs.
🤖 Generated with Claude Code