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
fix(core): hold shared instances fetched while runtime tests register - #7391
Tests that are built and registered while other tests run (AddDynamicTest, CreateTestVariant, and expanded deferred-enumeration placeholders) could receive a cached PerClass/PerAssembly/PerTestSession/Keyed instance, and the last running holder of that instance could then end before registration counted it. The holder then retired and disposed the instance, and the new test ran with a disposed object.
This PR makes registration take its reference before retirement can happen, which is the first option in the issue.
ObjectTracker.BeginRegistrationHolds() / ReleaseRegistrationHoldsAsync() bracket building plus registration in the three runtime paths (TestRegistry.ProcessPendingDynamicTests, TestRegistry.CreateTestVariantInternal, DeferredTestExpander.ExpandAsync). The holds live in an AsyncLocal, so they also reach Parallel.ForEachAsync registration workers.
TestDataContainer.GetOrCreate calls ObjectTracker.TryHoldForRegistration on each fetch in such a flow. The method takes s_sharedInstanceLock, re-checks IsCached, and increments the counter. This follows the same rule as TryHoldForRunningTest: an instance is either held or retired, never both. When the instance has just been retired, the fetch loops and the cache creates a new instance.
Once registration has counted its own references (in the finally), the holds are released. Releasing disposes only instances that no registered test kept, such as an instance created for a test whose registration failed. Before this change such an instance stayed alive until the session-end sweep.
When no holds are active, the fetch path skips everything after one Volatile.Read of an int, so discovery-time fetches pay almost nothing. Initial discovery is not bracketed, because no tests are running while it registers.
The issue's suggestion of re-resolving inside TrackObjects was not used. Class and method arguments are produced by the builder before registration, so there is nothing left to re-resolve at that point. Holding the instance at fetch time removes the race window entirely.
Tests
New tests in tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs, all deterministic:
Test_Registered_Between_The_Count_Reaching_Zero_And_Retirement_Keeps_The_Instance: uses the synchronous OnDisposed callback, as the issue suggests, to fetch between the decrement and the retirement.
TestProject net10.0, run as separate filters: Bugs._7204 (6/6 source-gen, 6/6 --reflection), DynamicTests/Runtime (8/8), TestVariantTests (10/10), TestVariantWithClassArgsTests (2/2), DeferEnumerationTests (42/42), DeferEnumerationClassDataTests (6/6)
TUnit.Engine Release build across all TFMs: 0 warnings
No public API change (all new members are internal)
Review fix (85020f6): a test that fails at registration now tracks the values that resolved before a sibling property threw, so releasing a registration hold cannot dispose them before its ITestEndEventReceiver runs. New Bugs._7391 regression: it fails without the fix, and 2/2 match expectations with it, in both source-gen and --reflection modes. Bugs._6554, _7213 and _7204 are unchanged in both modes, and UnitTests net10.0 passes 690/690 (4 skipped). PartialInjectionFailureDisposal7391Tests (Engine.Tests) runs in CI only.
Shared test instances remain available while dynamically generated and deferred tests are registered, preventing premature disposal.
If a shared instance has already been retired, a replacement is fetched. Instances not retained by a registered test are still disposed after registration.
Fetches made after registration has completed do not extend an instance’s lifetime.
Objects resolved before property or argument injection fails remain available through the failed test’s lifecycle, so test-end callbacks run before disposal.
Review fix (2b9f9e3): retain constructor/method arguments when argument property injection fails, and narrow the #6554 engine filter to its namespace. Added a deferred shared-argument regression; its observer fails without the fix and passes with it in source-gen and reflection modes. #7391 filter: 5 results (2 passing observers, 3 expected failures including the deferred container); #6554 filter: 2 results (1 pass, 1 expected failure), both modes. UnitTests net10.0: 691 passed, 3 skipped. Engine.Tests/AOT validation remains CI-only. Separate discoveries: #7409 and #7410.
No actionable comments were generated in the recent review.
Walkthrough
The change adds registration-scoped holds for cached shared instances and retries fetching when a hold cannot be acquired. Registration paths release holds after building and registering tests. Failed property and argument resolution now triggers tracking of objects resolved before the failure.
Changes: Shared Instance Registration
Layer / File(s)
Summary
Hold and retry shared instances src/TUnit.Core/Tracking/ObjectTracker.cs, src/TUnit.Core/TestDataContainer.cs, tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs
ObjectTracker adds registration-scoped holds and checks that an instance remains cached before holding it. TestDataContainer.GetOrCreate retries if it cannot hold a fetched instance. Five tests cover hold, retirement, and release behavior.
Apply holds during registration src/TUnit.Engine/Services/DeferredTestExpander.cs, src/TUnit.Engine/Services/TestRegistry.cs, src/TUnit.Engine/Framework/TUnitServiceProvider.cs
DeferredTestExpander and TestRegistry hold instances while building and registering tests, then release holds in finally blocks. TUnitServiceProvider passes ObjectTracker to both services.
Track objects after failed resolution src/TUnit.Engine/Services/ObjectLifecycleService.cs, src/TUnit.Engine/Services/TestArgumentRegistrationService.cs, tests/TUnit.TestProject/Bugs/_7391/*, tests/TUnit.Engine.Tests/PartialInjectionFailureDisposal7391Tests.cs, tests/TUnit.Engine.Tests/PartialInjectionFailure6554Tests.cs
The registration services track objects resolved before property or argument resolution fails, then rethrow the original error. Regression tests check callback and disposal order and the aggregate outcome. The _6554 test filter now targets its namespace.
thomhurst/TUnit#7394: Proposes replacing paired registration-hold calls with a scope; this PR adds the paired hold and release calls.
thomhurst/TUnit#7213: Reports objects not being retained when object-graph tracking fails. This PR tracks objects resolved before property or argument resolution failures.
Merge Risk: Minimal · up to 3472c
No actionable issue remains from this review; merge after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 25.00% 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 "fix(core): hold shared instances fetched while runtime tests register" matches the main change. The diff adds registration holds on cached shared instances (ObjectTracker.BeginRegistrationH…
Linked Issues check
Issue [#7263] requires registration to hold a cached shared instance before retirement or to resolve a new instance after retirement. TestDataContainer.GetOrCreate retries until `ObjectTracker.TryHo…
Out of Scope Changes check
The changed production files implement or support [#7263]. The partial property- and argument-injection tracking changes are required because registration-hold release can otherwise dispose values bef…
Full details: Title check
Explanation
The title "fix(core): hold shared instances fetched while runtime tests register" matches the main change. The diff adds registration holds on cached shared instances (ObjectTracker.BeginRegistrationHolds, TryHoldForRegistration, ReleaseRegistrationHoldsAsync), applies them in DeferredTestExpander, TestRegistry, and TestContainer fetches, and adds SharedInstanceLifetimeTests for the race. The title is concise, uses a conventional commit prefix, and names the specific fix. The supporting changes (property-injection failure disposal for _7391, the #6554 filter narrowing) are secondary and do not need to appear in the title.
Full details: Linked Issues check
Explanation
Issue [#7263] requires registration to hold a cached shared instance before retirement or to resolve a new instance after retirement. TestDataContainer.GetOrCreate retries until ObjectTracker.TryHoldForRegistration succeeds. The hold uses the shared-instance lock and checks the cache before it increments the reference. TestRegistry and DeferredTestExpander keep holds active through building and registration, then release them in finally blocks. The added deterministic tests cover the last-holder race, retirement between fetch and hold, retired-instance replacement, and post-release fetches. The partial-resolution tracking changes preserve resolved values until failed registration completes, which prevents the new release logic from disposing values too early.
Full details: Out of Scope Changes check
Explanation
The changed production files implement or support [#7263]. The partial property- and argument-injection tracking changes are required because registration-hold release can otherwise dispose values before a failed test's end event. The added #7391 tests verify this lifecycle behavior. The #6554 filter change only narrows the related regression test selector. No unrelated feature, public API change, or separate issue implementation appears in the reviewed changes.
Full details: Docstring Coverage
Explanation
Docstring coverage is 25.00% 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 12 functions across 12 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 guards the cached instance tight, While tests are built and held from flight. If fetching finds the old one gone, A fresh one joins the testing throng. On failure, resolved friends stay near, Then callbacks run before disposal here.
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: 2e6e0d53-e252-4a4a-8c05-c6e640cb5201
Commits
Reviewing files that changed from the base of the PR and between 2b9f9e3 and 3472c7e.
Files selected for processing: 1
src/TUnit.Core/Tracking/ObjectTracker.cs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
I found no blocking issues. The approach matches the first option in #7263: take the reference at fetch time, so the race window is closed rather than narrowed.
What looks right
TryHoldForRegistration takes s_sharedInstanceLock, re-checks IsCached and increments the counter. This follows the same "held or retired, never both" rule as TryHoldForRunningTest. GetOrCreate retries after a retirement, so the loop ends once a fresh instance is created.
The lock order is always holds.Instances then s_sharedInstanceLock. ReleaseRegistrationHoldsAsync only takes the holds lock, and only briefly, so I see no deadlock path.
The Released flag handles the case where the AsyncLocal still flows after release.
Fetches pay one Volatile.Read when no holds are active, so initial discovery is essentially unaffected.
The three runtime registration paths are all bracketed, and release is in a finally. Releasing now also disposes instances created for a test whose registration failed, which previously lingered until the session-end sweep.
The unit tests are deterministic. The OnDisposed callback test hits the count-zero-to-retirement gap directly.
Minor, non-blocking
BeginRegistrationHolds / ReleaseRegistrationHoldsAsync must be paired by hand in each call site, and the try/finally is repeated three times. A small IAsyncDisposable scope, for example await using var _ = _objectTracker.BeginRegistrationScope(), would make a missed release impossible and shorten the callers. It would also remove the odd mix of a static begin with an instance release. I'd consider it as a follow-up, not a blocker.
Holds last for the whole build phase. With very slow deferred data sources, disposal of the shared instances is delayed for that time. This is acceptable and is the cost of closing the race.
Holds last for the whole build phase: accepted as designed. Delaying disposal while a slow deferred source builds is the cost of closing the race, and the holds are released in the finally right after registration counts its own references.
Both reviewers approved head 3423c066ee with no blocking findings, so I'm marking this ready.
I found no blocking issues. The approach is sound: take the reference at fetch time, under s_sharedInstanceLock, using the same held-or-retired rule as TryHoldForRunningTest. That closes the race instead of narrowing it, and I agree with not re-resolving inside TrackObjects. The tests are deterministic, and the OnDisposed callback test hits the count-zero-to-retirement window directly. I read the diff only and did not run the tests.
Minor points, none blocking:
AsyncLocal scoping.BeginRegistrationHolds is a plain method that writes s_registrationHolds.Value. If the calling async method hasn't yet awaited, the value stays set in the caller's context. The Released flag makes this harmless, since later fetches skip released holds. But a nested Begin overwrites the outer holds without restoring them. Today the three call sites don't nest, but a future one could silently lose the outer holds. Two options would remove the footgun:
Return an IAsyncDisposable scope that restores the previous AsyncLocal value.
Add an assertion or a null check on entry.
Cost of the s_activeRegistrationHolds counter. The fast path is one Volatile.Read, which is fine. Because the counter is process-global, any concurrent registration makes every other flow's fetches also read the AsyncLocal. That is cheap, so this is just something to know about.
Release visibility.ReleaseRegistrationHoldsAsync is public on an internal class and reads holds.Instances.Count outside the lock. That is safe because Released is already set, but a short comment would help. Making it internal would be more consistent with the neighbouring members.
GetOrCreate retry loop. It has no bound. It terminates because a failed hold means the instance was evicted. If IsCached could ever stay false for a freshly created instance, for example an uncached factory result, this would spin. A defensive retry cap, or an assertion, would be cheap insurance.
No new actionable issues were found. Validation in this review was limited to reading code; no tests were run.
Diagram
sequenceDiagram
participant Builder as Runtime test builder
participant Cache as Shared instance cache
participant Tracker as ObjectTracker
participant Registration as Test registration
Builder->>Tracker: BeginRegistrationHolds()
Builder->>Cache: Fetch shared instance
Cache->>Tracker: TryHoldForRegistration(instance)
Tracker-->>Cache: Hold instance or retry fetch
Builder->>Registration: Register built tests
Registration->>Tracker: Count test references
Builder->>Tracker: ReleaseRegistrationHoldsAsync()
Note over Tracker: Registered tests keep their references until completion
…7263)
Review on #7391: releasing a registration hold could dispose a shared value
that a test failing at registration still exposes to its ITestEndEventReceiver.
Track what resolved before rethrowing, so the failed test holds it until it ends.
Also cover the refused registration hold, and make ReleaseRegistrationHoldsAsync
internal like its neighbours.
Process-global counter: acknowledged, no change. While any registration is active, fetches in other flows pay one extra AsyncLocal read.
ReleaseRegistrationHoldsAsync visibility: made internal in 85020f6. The comment above the unlocked Count read already explains why it is safe once Released is set.
Unbounded GetOrCreate retry: no change. A hold is refused only when IsCached is false, which means the instance was retired and evicted. The next Fetch then stores a fresh instance in the cache, and that instance cannot be retired before the hold is taken, because it has no tracked holder whose count could reach zero. A retry cap would only replace a real retirement race with a spurious failure.
85020f6 also fixes the Greptile P1 (values resolved before a sibling property throws are now tracked) and adds the refused-hold test from the Greptile P2.
The hot path is cheap. With no active holds it costs one Volatile.Read. That fits the allocation and performance constraints in AGENTS.md.
All three runtime registration paths use try/finally. The AsyncLocal carries the holds into Parallel.ForEachAsync workers.
The Released flag, checked under the same lock, stops late fetches in the same async flow from adding to a released set.
The TrackObjectsAfterFailedResolution change covers a real gap. Without it, releasing the hold would dispose a sibling property value before the failed test's end-event receivers ran.
The lock order (holds.Instances, then s_sharedInstanceLock) is consistent in what I read.
Suggestions (non-blocking)
The Begin/Release bracket is repeated in three places. A small helper such as ObjectTracker.RunWithRegistrationHoldsAsync(Func<Task>) would cut duplication and make a missing finally impossible. It would also remove the requirement in the doc comment that BeginRegistrationHolds be called from an async method. That requirement is easy to break when someone later refactors.
If the hold-failure loop in GetOrCreate keeps failing, it could spin. Please confirm every failed hold evicts the instance, so the next Fetch always makes progress. A comment or a debug assert would help.
Please confirm there is a regression test that fails without the fix: a dynamic test or variant created while the last holder of a PerClass instance ends. It should check that the new test never sees a disposed instance.
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
Inactive
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Summary
Tests that are built and registered while other tests run (
AddDynamicTest,CreateTestVariant, and expanded deferred-enumeration placeholders) could receive a cachedPerClass/PerAssembly/PerTestSession/Keyedinstance, and the last running holder of that instance could then end before registration counted it. The holder then retired and disposed the instance, and the new test ran with a disposed object.This PR makes registration take its reference before retirement can happen, which is the first option in the issue.
ObjectTracker.BeginRegistrationHolds()/ReleaseRegistrationHoldsAsync()bracket building plus registration in the three runtime paths (TestRegistry.ProcessPendingDynamicTests,TestRegistry.CreateTestVariantInternal,DeferredTestExpander.ExpandAsync). The holds live in anAsyncLocal, so they also reachParallel.ForEachAsyncregistration workers.TestDataContainer.GetOrCreatecallsObjectTracker.TryHoldForRegistrationon each fetch in such a flow. The method takess_sharedInstanceLock, re-checksIsCached, and increments the counter. This follows the same rule asTryHoldForRunningTest: an instance is either held or retired, never both. When the instance has just been retired, the fetch loops and the cache creates a new instance.finally), the holds are released. Releasing disposes only instances that no registered test kept, such as an instance created for a test whose registration failed. Before this change such an instance stayed alive until the session-end sweep.Volatile.Readof an int, so discovery-time fetches pay almost nothing. Initial discovery is not bracketed, because no tests are running while it registers.The issue's suggestion of re-resolving inside
TrackObjectswas not used. Class and method arguments are produced by the builder before registration, so there is nothing left to re-resolve at that point. Holding the instance at fetch time removes the race window entirely.Tests
New tests in
tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs, all deterministic:Test_Registered_While_The_Last_Holder_Ends_Keeps_The_InstanceTest_Registered_Between_The_Count_Reaching_Zero_And_Retirement_Keeps_The_Instance: uses the synchronousOnDisposedcallback, as the issue suggests, to fetch between the decrement and the retirement.Registration_Fetching_A_Retired_Instance_Gets_A_New_OneFetches_After_Holds_Are_Released_Are_Not_HeldWith the hold disabled, the first three fail. With it, they pass.
Validation
tests/TUnit.UnitTestsnet10.0: 689 succeeded, 0 failed (4 skipped)Bugs._7204(6/6 source-gen, 6/6--reflection),DynamicTests/Runtime(8/8),TestVariantTests(10/10),TestVariantWithClassArgsTests(2/2),DeferEnumerationTests(42/42),DeferEnumerationClassDataTests(6/6)TUnit.EngineRelease build across all TFMs: 0 warningsITestEndEventReceiverruns. NewBugs._7391regression: it fails without the fix, and 2/2 match expectations with it, in both source-gen and--reflectionmodes.Bugs._6554,_7213and_7204are unchanged in both modes, and UnitTests net10.0 passes 690/690 (4 skipped).PartialInjectionFailureDisposal7391Tests(Engine.Tests) runs in CI only.Closes #7263
Summary by CodeRabbit
Review fix (2b9f9e3): retain constructor/method arguments when argument property injection fails, and narrow the #6554 engine filter to its namespace. Added a deferred shared-argument regression; its observer fails without the fix and passes with it in source-gen and reflection modes. #7391 filter: 5 results (2 passing observers, 3 expected failures including the deferred container); #6554 filter: 2 results (1 pass, 1 expected failure), both modes. UnitTests net10.0: 691 passed, 3 skipped. Engine.Tests/AOT validation remains CI-only. Separate discoveries: #7409 and #7410.