[thomhurst]

Fixes #7204.

Problem

When the last test holding a shared data-source instance (PerTestSession, PerClass, PerAssembly or Keyed) ends, ObjectTracker disposes the instance, but TestDataContainer keeps it cached until the session ends. Any later request for the same scope gets the disposed instance back:

• a dynamic test registered after the original holders have finished (Shared data-source instances are reused after disposal by tests registered later in the session #7204);
• code that calls SharedDataSources.GetOrCreate itself, for example inside a test, after the holders have finished. That call also never took a reference, so the instance could be disposed while the test was still using it.

Fix

• Evict on retirement. TestDataContainer now keeps disposable cached instances in an identity index. When a cached instance's count reaches zero, ObjectTracker retires it: it removes the counter and evicts the instance under one lock, then disposes it. The next request creates a new instance.
• Running tests hold what they fetch. TestContext gets a hold window: it opens after the Before(Class/Assembly/Session) hooks and closes just before the test's tracked objects are untracked. A SharedDataSources.GetOrCreate call made by a test inside that window adds the instance to that test's tracked objects and takes one reference. The test then releases it when it ends, like an injected instance.
  • The window is closed during discovery and registration, so data sources that call SharedDataSources are still counted only by the object-graph walk. No reference is counted twice.
  • An instance the test already holds, for example through ClassDataSource, is not counted again.
  • Taking a reference and retiring an instance use the same lock, so an instance is either held or retired, never both. If it was retired first, GetOrCreate fetches again and gets a new instance.
  • Concurrent fetches inside one test are serialized on the test's tracked set.
• Docs: a new "When Shared Instances Are Disposed" section in class-data-source.md.

No public API changes (ScopedDictionary is untouched; TestDataContainer is internal).

Not covered

• Outside a running test (hooks, background code), GetOrCreate still does not take a reference. The docs now say so and point to static-property injection for session lifetime.
• GetOrCreate still does not call IAsyncInitializer.InitializeAsync; that would need a separate async API.
• A test that is registered (not fetching through SharedDataSources) at the same moment as the last holder ends can still race. This window existed before and is unchanged.

Tests

• tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs: eviction after disposal, holding until the test ends, a new instance after disposal, no double count, no hold outside the window, 32 concurrent fetches in one test. 4 of the 6 fail with the fix disabled; the other 2 guard against overcounting.
• tests/TUnit.TestProject/Bugs/_7204 + tests/TUnit.Engine.Tests/Issue7204Tests.cs: a dynamic test registered after disposal, and a running test fetching after disposal. Both fail without the fix, in source-gen and reflection modes.
• Full TUnit.UnitTests (623 passed, 4 skipped). Engine tests Issue7204, Issue7156, FilteredSharedFixtureDisposal, KeyedDataSource, DynamicTests, DynamicTestIndex, DerivedSkipWithClassDataSource, PartialInjectionFailure6554 and PropertyInjectionInitFailure pass.
• TestProject *Shared*, *Dispos*, *Inject* and *Keyed* subsets, both modes: failures are identical to main (intentional failures, plus the order-dependent ClassDataSourceSharedNoneNoLeakageTests counters, which also fail on main under these broad filters).

Found along the way

While writing the dynamic-test repro: TestRegistry.CreateRuntimeTestInvoker compiles the Expression<Action<T>>, so an async Task test method's task is discarded and its failures are never reported. Filed separately.

Summary by CodeRabbit

• Bug Fixes

  • Shared data source requests made after an instance is disposed now receive a new, live instance.
  • Instances fetched during a running test remain available until that test completes, including when fetched repeatedly or concurrently.
  • Instances fetched outside a running test are not held on behalf of the caller.

• Documentation

  • Added guidance on shared-instance lifetimes, including when instances are disposed, how later requests receive a new instance, and how to retain one for a test session.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-25T22:13:21.948348Z	c991548	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review.

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: e3d84c43-72a9-45a8-a9bc-b8f9b80e73fb

Commits

Reviewing files that changed from the base of the PR and between e3eb82d and c991548.

Files selected for processing: 7

• docs/docs/writing-tests/class-data-source.md
• src/TUnit.Core/TestContext.cs
• src/TUnit.Core/TestDataContainer.cs
• src/TUnit.Core/Tracking/ObjectTracker.cs
• tests/TUnit.Engine.Tests/Issue7204Tests.cs
• tests/TUnit.TestProject/Bugs/_7204/ConstructorFetchedSharedInstanceTests.cs
• tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs

Files skipped from review as they are similar to previous changes: 1

• docs/docs/writing-tests/class-data-source.md

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Walkthrough

Shared data-source instances are held during running tests and evicted from the cache when their tracked references reach zero. The changes add cache and reference-tracking logic, test coverage for disposal and retrieval, and documentation on shared-instance lifetime.

Changes: Shared instance lifetime

Layer / File(s)	Summary
Cache and retirement src/TUnit.Core/TestDataContainer.cs, src/TUnit.Core/Tracking/ObjectTracker.cs, tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs	A concurrent cache stores shared instances by scope and type. ObjectTracker coordinates reference acquisition, disposal, and cache eviction. Unit tests cover eviction, multiple keys, and retirement races.
Running-test reference holds src/TUnit.Core/TestContext.cs, src/TUnit.Engine/Services/TestExecution/TestCoordinator.cs, src/TUnit.Core/SharedDataSources.cs, src/TUnit.Core/Tracking/ObjectTracker.cs, tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs	Test execution defines when shared references can be held. SharedDataSources holds resolved instances for running tests and retries resolution if holding fails. Unit tests cover hold behavior and concurrent fetches.
Lifetime tests and documentation tests/TUnit.TestProject/Bugs/_7204/*, tests/TUnit.Engine.Tests/Issue7204Tests.cs, docs/docs/writing-tests/class-data-source.md	Regression tests cover retrieval after disposal, dynamic registration, and constructor-time fetching. Documentation describes shared-instance lifetime and access during and outside a test.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Low · up to c9915

Concurrent reuse of one shared instance under multiple keys can occasionally return a disposed instance. This is a bounded risk that can be accepted with owner awareness.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 9 files. (1 skipped: …	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 change: preventing disposed shared data-source instances from being returned.
Linked Issues check		Directly linked issue [#7204] requires that a test registered after the last holder of a shared instance gets a new instance, not the disposed cached one. Scopes: Keyed, PerClass, PerAssembly, PerTest…
Out of Scope Changes check		Changes and their link to #7204: - ObjectTracker.cs, TryRetireSharedInstance and RemoveCounter: eviction on retirement. This is the core fix. - ObjectTracker.cs, TryHoldForRunningTest: holds instances…

Full details: Linked Issues check

Explanation

Directly linked issue [#7204] requires that a test registered after the last holder of a shared instance gets a new instance, not the disposed cached one. Scopes: Keyed, PerClass, PerAssembly, PerTestSession.

Evidence at the reviewed head (c991548):

• src/TUnit.Core/Tracking/ObjectTracker.cs, UntrackObject: when the count of a cached instance reaches zero (TestDataContainer.IsCached), TryRetireSharedInstance runs. Under s_sharedInstanceLock it checks CurrentCount == 0, removes the counter, and calls TestDataContainer.Evict. Only then is the instance disposed. The outcome of the test does not affect this path.
• TryRetireSharedInstance and TryHoldForRunningTest use the same lock, so an instance is either retired or held. A held instance is not disposed. Re-hold after zero count is handled by the CurrentCount re-check.
• src/TUnit.Core/SharedDataSources.cs, GetShared: when TryHoldForRunningTest returns false (instance retired), the loop calls Resolve again. The cache then creates a new instance.
• Non-disposable instances are skipped by ShouldSkipTracking and are not indexed. Their caching behavior is unchanged.

Tests: the PR adds tests/TUnit.TestProject/Bugs/_7204 and Issue7204Tests (engine), plus SharedInstanceLifetimeTests (unit). I did not execute them. The reported pass results are author claims. The source evidence supports the fix for the reproduction in #7204.

Not covered by this PR and not required by the issue text: registration at the exact moment the last holder ends (documented by the PR as a pre-existing race), and async initialization through GetOrCreate. The issue does not request either.

Full details: Out of Scope Changes check

Explanation

Changes and their link to #7204:

• ObjectTracker.cs, TryRetireSharedInstance and RemoveCounter: eviction on retirement. This is the core fix.
• ObjectTracker.cs, TryHoldForRunningTest: holds instances fetched by a running test through SharedDataSources. This covers the same defect (a disposed shared instance handed out after disposal) through a second entry point. It is a related extension, not unrelated work.
• ObjectTracker.cs, UntrackObjectsAsync and UntrackBucketAsync: release test-held references last, and batch disposal per bucket with error collection. The release ordering supports the held-instance design. The batching is a refactor of the untracking path. The PR does not explain it separately, but it is part of the same untracking flow and is not a separate feature.
• ObjectTracker.TrackObjects, RemoveObjectsNotIn on discovery failure: removes only the entries added by the failed call. This prevents a test from releasing references it never took, which the eviction logic depends on.
• TestDataContainer.cs: identity index, IsCached, Evict, and Reset. These support eviction.
• SharedDataSources.cs, GetShared and Resolve: routes non-None sharing through one path that holds the instance for a running test. The old generic helpers were replaced by type-based helpers. Behavior for None is unchanged.
• TestContext.cs and TestCoordinator.cs: the hold window opens after Before(Class/Assembly/Session) hooks and closes before untracking. This bounds the hold behavior. The summaries describe these boundaries. I did not read these files directly.
• Docs (class-data-source.md): describes the new lifetime behavior.
• Tests (UnitTests, TestProject _7204, Issue7204Tests, ConstructorFetchedSharedInstanceTests): support the change.

I found no change that is demonstrably unrelated to shared-instance lifetime. The PR states that it makes no public API changes, and the summaries list none.

Full details: Docstring Coverage

Explanation

Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 9 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the cache at dawn,
And finds old shared instances gone.
While tests are running, holds stay tight,
Then disposal follows test-end night.
Fresh keys bring fresh instances to light,
The rabbit hops away, all right.}

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3eb82d826

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Review of #7248

I read the diff. I did not build or run the tests, and the code-review skill failed to run, so this was a manual read.

Overall: the fix looks sound and well-scoped. It addresses #7204 at the cause: the cache kept handing out an instance that ObjectTracker had already disposed.

What I checked

• Retirement: TryRetireSharedInstance does the count re-check, counter removal and eviction under s_sharedInstanceLock. TryHoldForRunningTest does the cached check and Increment under the same lock, so an instance is either held or retired, never both.
• Lock order: TryHoldForRunningTest takes the test's trackedObjects lock and then s_sharedInstanceLock. Retirement takes only the shared lock, so I see no inversion.
• StopHoldingSharedReferences: the Interlocked.Exchange followed by an empty lock is a valid way to drain in-flight adders before UntrackObjects. A short comment on why the empty lock is intentional would help, and the existing comment mostly does that.
• Retry loop in GetShared: it ends because eviction happens before TryHold can return false, so the next Resolve creates a new instance.
• Cache: replacing the four ScopedDictionary instances with one ConcurrentDictionary<InstanceKey, Lazy> and an identity index is a clean simplification. Removing the entry via ICollection.Remove(KeyValuePair) evicts only if the Lazy is still the same one, which avoids evicting a newer replacement.

Suggestions (non-blocking)

1. Cached exceptions: a Lazy with ExecutionAndPublication caches a factory exception forever. I assume the old ScopedDictionary behaved the same way, so this is not a regression. Check that it was equivalent, or use PublicationOnly / remove the entry on failure.
2. Reset(): it clears both dictionaries, but a counter for an instance that is still tracked would survive. IsCached would then be false, and untracking would take the plain RemoveCounter path. That is probably fine for a session-end reset, but a comment saying so would help.
3. Tests: the Issue7204Tests engine test polls with a 30s deadline. That is fine, but it is the likeliest source of flakiness under load. The unit tests in SharedInstanceLifetimeTests cover the race windows better, so lean on those.
4. Metadata modes: the change is in the shared execution path, so I don't see a need to duplicate it in the source-generated and reflection modes. Confirm CI covers both modes for the new engine test.
5. Public API snapshot: SharedDataSources signatures are unchanged and only private helpers were removed, so TUnit.PublicAPI should not change.

No blocking issues found. Nice work.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; all four previous findings are addressed.

Summary

Shared disposable instances are evicted when their last reference is released. Running tests hold instances they fetch until cleanup.

• Later requests get a new instance after a shared data source is disposed.
• A running test holds shared instances it fetches until its cleanup ends.

No new actionable issues were found. Tests were inspected, not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Test fetches shared instance] --> B{Already held by this test?}
    B -->|Yes| E[Return instance]
    B -->|No| C{Still cached under shared lock?}
    C -->|No| A
    C -->|Yes| D[Count reference and record runtime hold]
    D --> E
    E --> F[Test ends and stops new holds]
    F --> G[Release injected objects]
    G --> H[Release runtime holds]
    H --> I{Last reference?}
    I -->|Yes| J[Remove matching counter and evict all entries]
    J --> K[Dispose instance]

Loading

_{Reviews (2) · Last reviewed commit: "fix: address review findings on shared-i..." · Reviewed by Greptile}

[github-actions]

Review of #7248 (shared instance eviction on retirement)

I read the core changes in ObjectTracker, TestDataContainer and SharedDataSources. I did not run the tests, and I only skimmed TestContext and TestCoordinator. The design is sound: the identity index, the single lock for retire versus hold, and the RemoveCounter compare-and-remove all hold up. The GetShared retry loop is also correct, because a failed hold implies the instance was evicted. Docs and tests are included.

Concerns, in order of importance

1. A narrow race remains on the object-graph path. TrackObject (used by TrackObjects at registration and discovery) does GetOrCreateCounter(obj).Increment() without taking s_sharedInstanceLock.

   • Say a dynamic test fetched a cached instance, and the last holder then dropped it to zero. TryRetireSharedInstance removes the counter, evicts the instance and disposes it.
   • The registering test's TrackObject can then re-create a counter for the instance that was just disposed, or increment the stale counter it fetched earlier. The test ends up holding a disposed instance, which is the Shared data-source instances are reused after disposal by tests registered later in the session #7204 symptom, but only in a tiny window.
   • This is much smaller than the original bug, but the "either held or retired, never both" guarantee is only claimed for TryHoldForRunningTest. If you want the invariant to be real, route TrackObject for cached instances through the same lock and IsCached check. Otherwise, say in the docs or a comment that the guarantee covers only the hold window.

2. Lock ordering needs a note. TryHoldForRunningTest takes heldSharedInstances and then s_sharedInstanceLock. A one-line comment saying the order must never be reversed would help future changes.

3. TestDataContainer.Evict can leave a stale entry in _instances after a race. If one factory returns the same instance for several keys, the AddOrUpdate copy-on-write array is fine. But entries are removed from _instances only while the Lazy still matches, and _entriesByInstance is removed first. So IsCached can return false while a stale _instances entry still exists for a moment. It is harmless in practice, but worth knowing.

4. GetOrCreate does not call InitializeAsync. This is documented, but it is a surprising difference from ClassDataSource. Consider a short analyzer or XML-doc remark on the public methods themselves, not only in the docs page.

Verdict: a good fix with solid tests. I'd address item 1 or document its limit, and the rest are optional.