You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Preserve DI scope ownership and synchronize concurrent property registration - #7476
DI property factories can register scopes concurrently on the same events container. Unsynchronized collection creation and list/dictionary updates can lose cleanup ownership or fail registration, leaving externally owned services undisposed.
Extract the ownership and row-context foundation from issue-7308-events at a454169, without lifecycle delegate dispatch. Synchronize events/collection initialization and scope/object registration; clone, merge and cleanup use snapshots without holding monitors across awaits. Scope owners dispose once after retries, at their resolved objects’ graph depth, with completion covering failed factories and untracked values. Preserve constructor/method row state isolation, rejected-row cleanup and static/session ownership.
Add concurrent injected-property coverage (eight simultaneous resolutions with async initialization), 128 parallel DI factories, retry and alternating ownership disposal order, class-row cleanup, static cleanup and singleton accounting regressions. Update the old scope regression comments to describe the new ownership machinery.
UnitTests in generated and reflection modes: 780 passed, 3 skipped in each.
Both modes: ConcurrentDiPropertyTests (4), StateBagRowIsolationTests (4), DataScopeRetryLifecycleTests (1), NonDisposableDataScopeLifecycleTests (1), ScopeOwnedConstructorLifecycleTests (2), InjectedScopeDisposalOrderTests (2), MEDITest (1), all passed.
Targeted _7205 namespace in each mode: 14 passed, 3 skipped, 5 intentional failures (circular dependencies, invalid rows and a deliberately failing dependency); disposal observer passed.
Core netstandard2.0 build: zero warnings/errors. git diff --check passed.
Engine.Tests is reserved for CI and was not run locally. No shipped public signatures, generator implementations or production reflection paths changed. PublicAPI tuple-annotation snapshots were intentionally refreshed (the public tuple signature is unchanged).
Rebased onto main 374bb7c, preserving its shared-static leases and session ownership. Fixed the CI context-identity regression by resuming method enumeration with a fresh context, retaining the enumeration state baseline. Deferred discovery rows transfer their new scope collections into session cleanup; static scopes drain through the existing session cleanup path. Updated the row-holder regression to call the current internal API directly.
Additional validation on SDK 10.0.403 / macOS arm64:
UnitTests generated and reflection: 817 passed, 3 skipped each, including session isolation for deferred scope collections.
UniqueBuilderContextsOnEnumerableDataGeneratorTests: 2 passed in each mode; StateBagRowIsolationTests: 4 passed in each mode.
PublicAPI net10.0: 20 passed. net8.0/net9.0 runs rolled forward to the installed net10.0 runtime select net10 snapshots and therefore fail framework-header comparisons; their emitted Core API text matches the corresponding updated target snapshots. net472 remains CI-only.
StaticScopeCleanupTests passed after switching to the production session cleanup path; git diff --check passed. Engine.Tests remains CI-only.
Review follow-up: synchronized scope-collection replacement and preserved class-factory state for temporary constructors in both build paths. Added repeated constructor-state isolation coverage and extended singleton re-entry accounting. Full UnitTests: 817 passed / 3 skipped per mode; temporary constructor and row-isolation regressions: 4 passed each per mode; singleton regressions: 5 passed.
Review follow-up d7f5389: reflection static release drains private-context scopes even without callbacks or after failures. AsyncEvent publishes stable handler snapshots for concurrent registration/merge; duplicate delegate subscriptions retain distinct identities. Added failed/static/deferred cleanup and concurrent handler regressions. UnitTests: 823 passed / 3 skipped in each generated and reflection mode. Native AOT publish succeeded; Engine.Tests remains CI-only. Separate static DI metadata issue recorded on #7356.
Summary by CodeRabbit
Bug Fixes
Improved cleanup of dependency-injection scopes and services across test rows, retries, skipped tests, and session shutdown. Resources are released once, and cleanup continues when individual disposals fail.
Improved isolation of test state across generated rows and repeats, helping prevent state or cleanup from leaking between tests.
Improved reliability of event subscriptions and cleanup during concurrent operations.
Tests
Added coverage for resource lifetimes, state isolation, concurrent dependency injection, and cleanup failures.
No actionable comments were generated in the recent review.
Walkthrough
The changes add synchronized data-row scope ownership and disposal, propagate row-specific state and cleanup through test building, and complete scopes during test and session cleanup. New tests cover concurrency, retries, row isolation, deferred discovery, and static scopes.
Data-row scope collections associate resolved objects with scope owners and release associated scopes during object untracking. Event subscriptions publish immutable snapshots. Tests cover concurrent registration, ownership, retry lifetime, and disposal counts.
Row context and builder cleanup transfer src/TUnit.Core/TestBuilderContext.cs, src/TUnit.Engine/Building/TestBuilder.cs, tests/TUnit.UnitTests/ClassRowCleanupTests.cs, tests/TUnit.TestProject/StateBagRowIsolationTests.cs, tests/TUnit.TestProject/Bugs/_7205/DependencyInjectionScopeDisposalTests.cs
Builder contexts carry row-specific state and events through data generation. Unclaimed rows retain cleanup for failed, skipped, rejected, and lent rows. Tests cover state isolation and cleanup transfer in streaming and non-streaming builds.
sequenceDiagram
participant TestBuilder
participant TestContextEvents
participant DataRowScopeCollection
participant ObjectTracker
participant TestCoordinator
TestBuilder->>TestContextEvents: Register row data scopes
ObjectTracker->>DataRowScopeCollection: Release scopes for untracked objects
TestCoordinator->>DataRowScopeCollection: Complete remaining test scopes
TestContextEvents->>DataRowScopeCollection: Detach scopes for session cleanup
Loading
Possibly related PRs
thomhurst/TUnit#7328: Introduced builder-context event delivery, DI scope ownership, and cleanup for failed or unclaimed rows.
thomhurst/TUnit#7417: Includes row-context and data-row scope ownership work continued by this change.
thomhurst/TUnit#7384: Introduced row builder-context events and DI scope ownership used by these cleanup changes.
Merge Risk: Minimal · up to 5b924
No actionable issue was established for this change; it is mergeable after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 18.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main changes: preserving DI scope ownership and synchronizing concurrent property registration.
Linked Issues check
[#7472] The coding requirements remain met at head 5b92436. The incremental diff from d7f5389 could not be read because changed blobs are missing from the checkout. The head file src/TUnit.Core/…
Out of Scope Changes check
[#7472] The changes remain within the linked issue scope. The AsyncEvent changes support the required clone and merge foundation for cleanup handlers. The nullable annotation fix at head 5b92436 ad…
Full details: Linked Issues check
Explanation
[#7472] The coding requirements remain met at head 5b92436. The incremental diff from d7f5389 could not be read because changed blobs are missing from the checkout. The head file src/TUnit.Core/AsyncEvent.cs was read directly and confirms the stated design. Add, AddAt, and AddMissingFrom copy the handler list under a lazily initialized lock and publish it with Volatile.Write. InvocationList and Clone read the published snapshot without a monitor. AddMissingFrom deduplicates by Invocation reference identity, so separate subscriptions of the same delegate remain distinct. No lifecycle delegate dispatch was added. The prior assessment of the DataRowScopeCollection, DependencyInjectionDataSourceSourceAttribute, TestContextEvents, ObjectTracker, TestBuilder, TestCoordinator, and TestOnDisposeRaiser changes is preserved, because the objectives state that the latest follow-up changed only AsyncEvent nullable annotations and the stable handler snapshots. The reported UnitTests, targeted tests, and Native AOT validation are author-reported and were not re-run here. The CI verdict for head 5b92436 was still pending in the supplied evidence.
Full details: Out of Scope Changes check
Explanation
[#7472] The changes remain within the linked issue scope. The AsyncEvent changes support the required clone and merge foundation for cleanup handlers. The nullable annotation fix at head 5b92436 addresses the CS8602 build errors and does not change the public signature. The PublicAPI snapshot updates document annotation output only. Test changes cover the linked ownership, concurrency, row-isolation, static cleanup, and singleton accounting requirements. The PR does not add lifecycle delegate dispatch, which the issue assigns to a sibling issue. The separate static DI metadata issue [#7356] is recorded but not implemented here. The incremental diff could not be read, so this verdict relies on the head AsyncEvent.cs read and the supplied PR evidence.
Full details: Docstring Coverage
Explanation
Docstring coverage is 18.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 83 functions across 22 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each scope at night, And keeps each row’s state tucked right. The handlers line up in a row, While cleanup waits for tests to go. Each owner rests once, safe and sound.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: f242d1b4-0c99-44e8-8e65-16e65aa2a89c
Commits
Reviewing files that changed from the base of the PR and between d7f5389 and 5b92436.
Files selected for processing: 1
src/TUnit.Core/AsyncEvent.cs
Files skipped from review as they are similar to previous changes: 1
src/TUnit.Core/AsyncEvent.cs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
The design is sound. Snapshot-then-act is the right pattern here: DataRowScopeCollection copies owners and objects under a short lock and disposes outside it, so no monitor is held across an await. AddRange also snapshots the source before locking the destination, which avoids lock-order inversion between two collections. I read the source changes only (the new collection, TestContextEvents, ObjectTracker, TestBuilderContext, and the TestBuilder hunks I got through). I did not read the test files or run anything.
Strengths:
ScopeOwner uses Interlocked.Exchange, so disposal is attempted once even when it throws. Cloned rows can safely share owners.
TestBuilderContext.CreateChild replaces three hand-rolled context constructions in TestBuilder, which makes the state and events isolation rules explicit with shareFactoryContext.
The test coverage matches the failure modes described: concurrent resolution, retries, alternating ownership order, static and class-row cleanup.
Points to consider (none blocking):
Unsynchronized OnDispose creation.TestContextEvents.AddCleanupFrom runs (OnDispose ??= new()).AddMissingFrom(...) outside _scopeLock. OnDispose is a plain auto-property. If two factories merge cleanup into the same events container concurrently, one AsyncEvent can overwrite the other and lose handlers. That is the same class of race this PR fixes for scopes. Moving it into the existing lock, or using LazyInitializer.EnsureInitialized, would close it. I also couldn't tell from the diff whether AsyncEvent.AddMissingFrom is itself thread-safe. If it isn't, wrapping it in the same lock covers both.
Concurrent callers of ScopeOwner.DisposeAsync. A second caller returns immediately while the first is still disposing. If anything relies on CompleteAsync meaning "everything is disposed" (for example, before reporting test end), a racing caller could proceed early. Caching a Task or ValueTask for the first disposal and awaiting it from later callers would give true once-only completion semantics. It would also surface the first exception consistently rather than only to the first caller. It is probably fine if releases are strictly sequential per row, but that assumption should be commented.
Disposal depth for multi-object owners.ReleaseAsync disposes every owner registered for the object being released, so an owner registered against several objects is disposed at whichever object is untracked first. The summary says the first resolved object's depth. If that is intended, a short test pinning the multi-object case would help, since RegisterObject supports it.
Cost of snapshots.AddRange and CompleteAsync allocate arrays and LINQ for every clone. That is cold relative to test execution, but the repo guidelines favour avoiding LINQ in engine paths. Handing out an immutable copy-on-write array, or using a plain loop, would be marginally cheaper.
s_hasExternallyOwned never resets. The comment justifies this, and it only costs a skipped fast path. It is fine.
Overall this is a clear improvement with good regression tests. Item 1 is the one I would address before merge.
[High impact] The PR appears safe to merge; no new blocking issue was found.
Summary
The PR preserves DI scope ownership through test rows, retries, and session cleanup. It also isolates row state and protects concurrent scope registration.
Dependency-injection scopes stay with their row objects until the right cleanup point.
Concurrent property factories keep their event and scope registrations.
Each class and method row keeps its own state and cleanup.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DI factory creates scope] --> B[Register scope before resolving objects]
B --> C[Register resolved objects]
C --> D{Who owns the row?}
D -->|Executable test| E[Keep scope alive through retries]
E --> F[Release scope during object cleanup]
F --> G[Complete remaining scopes]
D -->|Discovery or static context| H[Transfer cleanup to session]
H --> I[Release scopes at session end]
Addressed Claude item 1 in d42cc36: AddCleanupFrom now holds the events container's existing lock across both OnDispose creation and AddMissingFrom, protecting the destination handler list as well as initialization. Added 128 concurrent cleanup merges, merging each source twice and verifying every callback runs exactly once.
Validation: DataRowScopeCollectionTests passed all 5 tests on SDK 10.0.403 / net10.0 in generated and reflection modes; git diff --check passed. Engine.Tests was not run locally, per repository instructions.
Disposition of the remaining nonblocking observations:
Item 2: retain once-only disposal-attempt semantics. ObjectTracker awaits each ReleaseAsync before advancing, and TestCoordinator awaits graph untracking before CompleteAsync. This change does not add a concurrent disposal completion contract.
Item 3: disposing at the first released resolved object's graph depth is intentional, as documented on DataRowScopeCollection; no production change requested.
Item 4: retain snapshots to avoid monitor ownership across awaits and lock-order inversion. No measured performance regression justifies changing that design in this fix.
Item 5: retain the documented monotonic externally-owned fast-path flag.
The new head remains draft pending fresh reviewer verdicts.
I read DataRowScopeCollection and TestContextEvents in full. I did not read the other changed files or the tests, and I ran nothing. The design is sound. Locks are only held for short, synchronous sections, and snapshots are taken before any await. AddRange snapshots the source before locking the destination, so there is no lock-order inversion. ScopeOwner disposes once through Interlocked.Exchange, and the exception is still recorded on the first attempt.
Points worth considering:
Remaining unsynchronized access to OnDispose.AddCleanupFrom does OnDispose ??= new() under _scopeLock. OnDispose is a public settable auto-property, and Clone(), user code and other engine code touch it without that lock. The PR description says the problem was concurrent DI factories registering on the same events container. If those factories can also call events.OnDispose ??= ... (the usual pattern), the lost-update race is still there. A small GetOrCreateOnDispose() helper using the same lock would close it. Alternatively, make the lazy-init helpers the only supported path internally and document that.
Lock object created lazily.LazyInitializer.EnsureInitialized(ref _scopeLock) on every call is safe. A private readonly object _scopeLock = new() costs about 24 bytes per TestContextEvents and removes the repeated lazy-init calls and the !. If per-test allocation is the concern, a comment saying so would help.
RegisterObject uses List.Contains per owner. This is fine for small lists. A single object resolved by many scopes is unlikely, so I don't see a real problem. AddRange re-enters RegisterObject while holding _lock, which works because the lock is reentrant. A short comment would stop someone from later replacing the lock with a non-reentrant primitive.
Clone() consistency.TestContextEvents.Clone reads DataRowScopes and OnDispose without _scopeLock. A concurrent AddCleanupFrom could produce a clone with scopes but not the matching dispose handlers. This is probably unreachable if cloning happens before execution. Taking the lock for the clone would make that safe regardless of ordering.
The tests listed in the PR description (8 concurrent resolutions, 128 parallel factories, retry and ordering regressions) cover the intended behaviour well. Overall this looks good, and none of the points above should block the merge.
Reviewed all conversation comments, reviews and inline threads for d42cc36; there are no review threads. Claude and Codex both give positive verdicts for this head.
Disposition of Claude’s latest nonblocking observations: retain the current implementation. The internal OnDispose initialization sites are the locked AddCleanupFrom and the sequential rejected-row Return path in TestBuilder; DI scope registration uses GetOrCreateDataRowScopes rather than adding OnDispose handlers. Public event mutation does not gain a general concurrent mutation contract here. Clone callers snapshot enumeration/row contexts before execution; concurrent cloning with user event mutation is outside that lifecycle. The lazy lock avoids allocating a monitor for events containers that never register scopes. RegisterObject’s small owner lists and reentrant monitor are intentional; no alternative primitive is introduced.
Marking ready to start CI and the ready_for_review Claude run. Auto-merge must wait for that fresh Claude verdict under the repository gate.
I read the source changes and skimmed the tests. I didn't build or run anything.
What looks good
DataRowScopeCollection is a clear abstraction. Cleanup ownership sits in a once-only ScopeOwner, and registration happens before the factory runs, so a failed factory still releases its scope.
Locking is done properly: AddRange snapshots the source before taking the destination lock, which avoids lock-order inversion. No monitor is held across an await in ReleaseAsync, CompleteAsync or DisposeAsync.
TestBuilderContext.CreateChild replaces four hand-rolled copies of context construction in TestBuilder. That removes a class of state-bag and events drift bugs.
Coverage targets the real risks: concurrent injected properties, 128 parallel DI factories, retry, alternating disposal order, class-row cleanup, and static and singleton accounting.
Suggestions (non-blocking)
Lock model:TestContextEvents now uses a lazily created _scopeLock object plus LazyInitializer, while DataRowScopeCollection has its own lock. AddCleanupFrom locks the destination's _scopeLock but reads source.DataRowScopes and source.OnDispose without the source's lock. AsyncEvent.AddMissingFrom iterates source.InvocationList the same way. If the source can still be mutated concurrently, a snapshot taken under the source's lock would be safer. Alternatively, document that sources are quiescent at that point.
Allocation in a hot path:AddRange allocates an array per owner via LINQ Select/ToArray. Row cloning happens per test, so a pooled or struct-based snapshot would help. This is a minor point unless benchmarks show it.
TestBuilder complexity: the activeClassRow, classEnumerationContext and ReferenceEquals(contextAccessor.Current, ...) checks in the catch blocks are hard to follow. Moving the unclaimed-row and failure-context state machine into UnclaimedClassRow, with a single GetFailureContext(), would make the invariants testable on their own.
Disposal exceptions:DisposeAsync marks the owner disposed before disposing, so an exception is surfaced once and never retried. That matches the comment, and I think it's the right call.
Overall this is a sound fix for the concurrency bug in #7472. The remaining points are maintainability items. Please make sure CI (Engine.Tests, PublicAPI and AOT) is green before merging.
Run 41616065913 passed; that earlier wait is cleared. Pushed e167fcf after rebasing onto main, resolving session/static cleanup conflicts, fixing the failing enumeration-context identity regression and refreshing Core tuple-annotation snapshots without changing public signatures. Session scope isolation and static cleanup regressions exercise the reconciled production paths.
Validation: UnitTests 817 passed / 3 skipped in generated and reflection modes; context identity 2 and row isolation 4 passed per mode; PublicAPI net10.0 all 20 passed. Native AOT publish succeeded and targeted context identity (2), row isolation (4), concurrent DI (4) passed. Full Engine.Tests and net472 validation remain CI-only. See the updated PR body for the net8/net9 roll-forward snapshot comparison limitation.
Disposition of the ready-review nonblocking suggestions: source enumeration contexts are quiescent when cleanup is merged; retain snapshots and the current row-state abstraction without an unmeasured allocation redesign. Disposal-attempt semantics remain intentional. No review threads exist. The PR is draft again for fresh Claude and Codex verdicts on this new head; no auto-merge is armed.
I read the diff but did not build it or run the tests.
What looks good
DataRowScopeCollection handles the concurrency cleanly. It snapshots under the lock and awaits outside it. AddRange snapshots the source before it locks the destination, which avoids lock-order inversion.
ScopeOwner.DisposeAsync is dispose-once through Interlocked.Exchange, including when disposal throws. That is the right behaviour for scopes shared between cloned rows.
Registering the scope before resolving means a failed factory still releases its scope.
TestBuilderContext.CreateChild replaces several hand-written copies of the context setup in TestBuilder. That removes duplicated state-bag and event wiring, which is where the earlier bugs came from.
The coverage matches the description: concurrent injected properties, retries, disposal order and rejected rows.
Concerns
TestContextEvents.DataRowScopes has a lock-free setter (Volatile.Write). GetOrCreateDataRowScopes and DetachDataRowScopes use _scopeLock. Because the setter bypasses the lock, a concurrent GetOrCreate can create a collection that a later write silently replaces. Make the setter private, or route every write through the lock.
AsyncEvent.AddMissingFrom reads source.InvocationList and mutates _handlers without synchronization. AddCleanupFrom guards the destination with _scopeLock, but the source's OnDispose can still be modified concurrently. It would be safer to snapshot the source under its own lock, or to document that the source is quiescent at that point.
TestBuilder.BuildTestsFromMetadata gained a lot of context-swapping state: activeClassRow, classSourceContext, classRowEnumerationContext, enumerationContext, and a new catch block. It is hard to check that every path restores contextAccessor.Current. A small scoped helper that restores the previous context on exit would make that guarantee structural instead of relying on manual resets. Several of the new UnclaimedClassRow methods (TransferCleanupTo, CreateEnumerationFailureContext, TakeLentObjects) have overlapping responsibilities. Consolidating them into one ownership-transfer API would reduce the chance of cleanup being claimed twice or dropped.
The ObjectTracker change attaches ownership by weak identity mark for externally-owned singletons, and the flag stays set permanently. That is fine, but it deserves a test for the case where a singleton is supplied by two sources after its count has reached zero.
Overall the direction is sound and the concurrency fix is well targeted. I'd address point 1 before merging and consider the refactor in point 3 as a follow-up.
Run 41616065913 completed successfully; the earlier wait is cleared. Review fixes are pushed as 67737e2.
Disposition of the current Claude review:
Fixed: DataRowScopes writes now take the same lock as GetOrCreate and Detach, retaining volatile reads.
Documented the source-quiescence requirement at AddCleanupFrom. Saved row/enumeration sources finish registering cleanup before transfer; destination registration is synchronized. AsyncEvent's public event mutation contract is unchanged.
Retain the existing row ownership/context restoration structure for this fix. The concern is a nonblocking maintainability suggestion; a broad ownership refactor would enlarge a concurrency correction without evidence of another incorrect path. The temporary-constructor context defect identified by Greptile is fixed in both paths, and its thread is resolved.
Extended Counted_Between_The_Check_And_The_Mark_Is_Released_Without_Being_Disposed to cover two later consumers after the previous count reaches zero, asserting shared accounting, balanced release and no disposal.
Validation on SDK 10.0.403 / net10.0: TemporaryConstructorRowStateTests and StateBagRowIsolationTests each passed 4 tests in generated and reflection modes. ExternallyOwnedSingletonTests passed 5 tests. Full UnitTests passed 817 with 3 skipped in each mode; git diff --check passed. Engine.Tests remains CI-only. No generator output, shipped public API or reflection mechanism changed. Build warnings already tracked in #7410, #7428 and #7438 remain outside this fix.
The PR remains draft for fresh Claude and Codex verdicts on the new head; no auto-merge is armed.
Run 41661284541 completed successfully on 5b92436; the earlier workflow wait is cleared. The ready-state Claude verdict explicitly finds no blocking problems. Codex records completed review of this full head with no findings. All reviews, inline comments, review threads and PR conversation comments have been read; both inline threads are resolved without later replies.
Disposition of the latest optional Claude suggestions: retain the documented first-resolved-object graph-depth release semantics rather than introduce last-object reference counting. DataRowScopeCollection and RegisterObject explicitly document this behavior; changing it would alter the ownership policy already covered by ordering regressions. Retain finite owner-list transfers and once-only disposal; duplicate entries do not cause duplicate disposal. Retain Contains-based merging for small handler lists without an unmeasured optimization. Retain the established outer events lock followed by the collection lock; no inverse acquisition was identified. These suggestions were explicitly nonblocking. CodeRabbit's generic docstring-coverage warning is advisory; retain the focused ownership documentation instead of adding boilerplate across tests and internal helpers.
All current CI is green, including Engine tests and AOT on Linux, Windows and macOS. Branch protection has been checked directly. The PR delivers #7472 and already includes Closes #7472. No prerequisite, superseding PR, remaining issue scope or owner decision prevents merge. Proceeding through Merge-Pr.ps1 with the reviewed head unchanged.
Review of #7476: DI scope ownership and row-context foundation
I reviewed the core changes: AsyncEvent, DataRowScopeCollection, TestContextEvents, ObjectTracker, TestSessionContext and StaticPropertyReflectionInitializer. I did not read the TestBuilder.cs changes (about 800 lines of the diff) or the new tests in detail, and I did not run anything. Overall the design is sound and I found no blocking problems.
What works well
AsyncEvent uses copy-on-write immutable snapshots, so readers, Clone and InvocationList need no lock. No monitor is held across an await anywhere I looked.
DataRowScopeCollection.AddRange snapshots the source before taking the destination lock, which avoids lock-order inversion.
ScopeOwner guards disposal with Interlocked.Exchange, so cloned rows and retries dispose a scope once. Registering the scope before the factory resolves means a failing factory still releases it.
The static and session paths now drain DataRowScopes separately from OnDispose: StaticPropertyReflectionInitializer drains them even when initialization failed, and DeferOnDisposeToSessionEnd carries them across. That covers the gap created by moving DI cleanup off OnDispose.
Tests are broad: concurrency, retry, ordering, class-row cleanup, static cleanup and singleton accounting.
Suggestions and questions (non-blocking)
Disposal depth for multi-object scopes.ReleaseAsync disposes the whole scope when the first resolved object is released, which is the deepest object in cleanup order. A shallower object from the same scope, such as the test class, may still be alive and may use scoped services in its own DisposeAsync. The InjectedScopeDisposalOrderTests cases may cover this. If not, consider ref-counting the owner by registered object (dispose when the last registered object is released), with CompleteAsync as the backstop. That would match "dispose at the graph depth of its resolved objects" more literally and avoid early disposal.
AddRange growth._owners.AddRange(owners) never de-duplicates, whereas RegisterObject does. Repeated clone/merge across retries or rows can accumulate duplicate owners. This is harmless for correctness because of the once-only guard, but a HashSet or a Contains check would keep it bounded.
AsyncEvent.AddMissingFrom.handlers.Contains makes the merge O(n²). Handler counts are small, so this is fine today. If you want a cheap guard, check by reference with a HashSet<Invocation>.
Two lock layers in TestContextEvents._scopeLock serializes scope initialization, AddCleanupFrom and DataRowScopes assignment, while DataRowScopeCollection has its own lock. The nesting always goes outer to inner, so it is safe. A short comment stating that order would help future edits.
Nothing here should block the merge. Thanks for the thorough validation notes.
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
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.
GitHub Actions
Failure
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
DI property factories can register scopes concurrently on the same events container. Unsynchronized collection creation and list/dictionary updates can lose cleanup ownership or fail registration, leaving externally owned services undisposed.
Extract the ownership and row-context foundation from
issue-7308-eventsat a454169, without lifecycle delegate dispatch. Synchronize events/collection initialization and scope/object registration; clone, merge and cleanup use snapshots without holding monitors across awaits. Scope owners dispose once after retries, at their resolved objects’ graph depth, with completion covering failed factories and untracked values. Preserve constructor/method row state isolation, rejected-row cleanup and static/session ownership.Add concurrent injected-property coverage (eight simultaneous resolutions with async initialization), 128 parallel DI factories, retry and alternating ownership disposal order, class-row cleanup, static cleanup and singleton accounting regressions. Update the old scope regression comments to describe the new ownership machinery.
Closes #7472
Validation on SDK 10.0.403 / net10.0:
_7205namespace in each mode: 14 passed, 3 skipped, 5 intentional failures (circular dependencies, invalid rows and a deliberately failing dependency); disposal observer passed.git diff --checkpassed.Engine.Tests is reserved for CI and was not run locally. No shipped public signatures, generator implementations or production reflection paths changed. PublicAPI tuple-annotation snapshots were intentionally refreshed (the public tuple signature is unchanged).
Rebased onto main 374bb7c, preserving its shared-static leases and session ownership. Fixed the CI context-identity regression by resuming method enumeration with a fresh context, retaining the enumeration state baseline. Deferred discovery rows transfer their new scope collections into session cleanup; static scopes drain through the existing session cleanup path. Updated the row-holder regression to call the current internal API directly.
Additional validation on SDK 10.0.403 / macOS arm64:
Review follow-up: synchronized scope-collection replacement and preserved class-factory state for temporary constructors in both build paths. Added repeated constructor-state isolation coverage and extended singleton re-entry accounting. Full UnitTests: 817 passed / 3 skipped per mode; temporary constructor and row-isolation regressions: 4 passed each per mode; singleton regressions: 5 passed.
Review follow-up d7f5389: reflection static release drains private-context scopes even without callbacks or after failures. AsyncEvent publishes stable handler snapshots for concurrent registration/merge; duplicate delegate subscriptions retain distinct identities. Added failed/static/deferred cleanup and concurrent handler regressions. UnitTests: 823 passed / 3 skipped in each generated and reflection mode. Native AOT publish succeeded; Engine.Tests remains CI-only. Separate static DI metadata issue recorded on #7356.
Summary by CodeRabbit
Bug Fixes
Tests