fix: dispose shared fixtures when only a subset of consuming tests runs

[thomhurst]

Problem

Discussion #6151: a [ClassDataSource<T>(Shared = SharedType.PerTestSession)] fixture implementing IAsyncDisposable never gets DisposeAsync called when running a single test from Rider (one of two [Arguments] cases).

Root cause

Shared-object disposal is pure ref-counting (ObjectTracker): +1 per test that registers the object, -1 when a test finishes, dispose at 0.

The increment happened at build time (TestBuilder.BuildTestAsync -> RegisterTestArgumentsAsync), but filtering happens after building, and decrements only happen for executed tests. Any built-but-not-run test permanently inflated the count, so it never reached zero:

• A single [Arguments] case selected by an IDE uid filter (both cases share one metadata entry, so both are always built) - the reported repro
• An [Explicit] sibling matched-then-excluded by a filter
• Discovery-only requests (IDE server mode), which built and registered all tests - also meaning discovery constructed expensive fixtures (e.g. WebApplicationFactory) as a side effect

Fix

1. Move argument registration post-filter. Removed the build-time RegisterTestArgumentsAsync call; TestFilterService.RegisterTest (which already performed the same call with identical failure semantics) is now the single registration point, running only for tests that will execute (incl. dependency-expanded ones).
2. Discovery-only requests skip argument registration (event receivers still run, so skip reasons / display-name formatters are unaffected). Discovery no longer creates shared fixtures at all.
3. Dynamic tests register explicitly. AddDynamicTest / CreateTestVariant bypass the discovery pipeline, so TestRegistry now registers built tests before enqueueing them (this also gives them OnTestRegistered receivers, which previously never fired for dynamic tests).
4. Session-end sweep + cache reset. At the end of each run session, any still-tracked objects are disposed (covers cancellation/miscounts) and TestDataContainer is cleared, so a subsequent run request in the same process (IDE server mode) creates fresh fixtures instead of receiving disposed ones. Runs after After(TestSession) hooks and static-property disposal - existing ordering preserved.

ObjectTracker.ClearStaticTracking previously had zero callers; the new sweep wires that lifecycle up properly.

Tests (TDD - red first, then green)

• TUnit.TestProject/Bugs/6151/Bug6151FilteredDisposalTests.cs - PerTestSession fixture with a normal test + [Explicit] sibling (same built-but-not-run shape as the IDE uid-filter case, reproducible via --treenode-filter). A guarded [After(TestSession)] hook writes created/disposed counts to a marker file (opt-in via env var, inert otherwise).
• TUnit.Engine.Tests/FilteredSharedFixtureDisposalTests.cs - asserts Created=1;Disposed=1 for the filtered-subset run (failed with Disposed=0 before the fix) plus a direct-single-test sanity case.
• InvokableTestBase.RunOptions gained WithEnvironmentVariable to support the marker handshake.

Verification

• New regression test red before fix (Created=1;Disposed=0), green after - both source-gen and reflection modes
• TUnit.UnitTests: 218/218
• Engine-test batch (dynamic, explicit, skip, retry, cancellation, data-source, property-injection, session/discovery hooks, disposal): 35/35 (AOT variants run in CI)
• TestProject disposal suites both modes: Bugs 1924 (660 tests), 5982, 2867, SharedDisposalTest, DisposalRegressionTests, NestedDisposalOrder - all green
• TUnit.PublicAPI snapshots updated for the two new Clear() members (ScopedDictionary, ThreadSafeDictionary)

Note: ClassDataSourceTupleTests.Test_ClassDataSource_UnwrapsTuples fails identically with and without this change (verified via stash) - pre-existing, unrelated.

Fixes #6151

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
CodeStyle	1 minor

View in Codacy

🟢 Metrics 4 complexity

Metric	Results
Complexity	4

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This is a well-motivated fix with a clear root cause analysis. The architecture change — moving argument registration from build-time to post-filter — is correct, and the TDD approach (red-first test) gives good confidence in the regression coverage. A few observations and concerns follow.

---

Double-disposal risk for static properties

The most significant concern is in TestSessionCoordinator.ExecuteTests. The session-end sweep calls DisposeAndClearStaticTrackingAsync, which iterates s_trackedObjects and disposes everything still tracked. Static-property objects are added to s_trackedObjects via StaticPropertyHandler.TrackStaticProperties (called from TestScheduler.ScheduleAndExecuteAsync), and they are explicitly disposed by StaticPropertyHandler.DisposeStaticPropertiesAsync later in that same ScheduleAndExecuteAsync call — but DisposeStaticPropertiesAsync disposes directly via _disposer.DisposeAsync, bypassing UntrackObject (i.e. it does not decrement the counter or remove the entry from s_trackedObjects).

That means static-property objects will still be present in s_trackedObjects when the sweep runs in the finally block of ExecuteTests, and DisposeAndClearStaticTrackingAsync will call Disposer.DisposeAsync on them a second time. Whether this causes harm depends entirely on whether user-provided disposable objects are idempotent — which the framework cannot assume.

If the ClearStaticTracking path existed previously and was never called, one might argue this was always a latent issue, but the PR is what introduces the first caller, so it owns the risk. The fix should either:

• Have DisposeStaticPropertiesAsync go through UntrackObject instead of calling _disposer.DisposeAsync directly, so the counter reaches zero and the entry is removed before the sweep runs, or
• Skip objects that are already zero-ref-counted (the counter exists but is at 0) in DisposeAndClearStaticTrackingAsync.

---

DisposeAndClearStaticTrackingAsync is an instance method on a class with only static state

ObjectTracker.DisposeAndClearStaticTrackingAsync operates entirely on s_trackedObjects and s_asyncCallbackErrors, both of which are static. The method is non-static only because it captures the disposer constructor parameter. This is an existing asymmetry (ClearStaticTracking is already static), but the new method deepens the confusion: callers that hold an ObjectTracker instance see a method that mutates all instances' shared state. A brief comment on why this is intentionally an instance method (or making disposer static if that's feasible) would prevent future confusion.

---

TestContext.Current = null removal leaves a gap

In the original BuildTestAsync, after calling RegisterTestArgumentsAsync, TestContext.Current was cleared to null. That reset was inside the finally block so it ran regardless of success or failure. The PR removes RegisterTestArgumentsAsync from BuildTestAsync but also removes that TestContext.Current = null reset entirely — there is no longer a finally clause at all. If anything sets TestContext.Current during BuildTestAsync (e.g. the DiscoveredTest assignment at line 76 in the pre-PR code), the context could leak into the next build iteration. Worth verifying that TestContext.Current is reliably reset at the right point on the new code path.

---

DiscoverTestsFullyStreamingAsync does not call RegisterTestsAsync

TestDiscoveryService.DiscoverTestsFullyStreamingAsync (used by the streaming discovery path) calls InvokePostResolutionEventsInParallelAsync and then yields tests to the caller, but never calls _testFilterService.RegisterTestsAsync(...). DiscoverTests (the non-streaming path) does call it. If DiscoverTestsFullyStreamingAsync is used in any execution scenario (currently it appears to only be used for IDE streaming discovery, isForExecution: false), those tests would not get their argument registration. The PR comment in DiscoverTests line 106 notes isForExecution drives the guard, so this is probably fine today, but it's worth a defensive assertion or comment to make the invariant explicit.

---

SessionSharedFixture static counters are not reset between runs in the same process

Bug6151FilteredDisposalTests uses SessionSharedFixture.CreatedCount and DisposedCount as static int fields. TestDataContainer.Reset() clears the instance caches so fresh objects are created, but CreatedCount/DisposedCount are user-defined statics that are not reset. If the engine test runs against a long-lived process that has already executed these tests in a previous session, the marker file would contain cumulative counts rather than per-run counts, and the assertion "Created=1;Disposed=1" would fail. This is a testing-infrastructure concern rather than a production bug, but the test's reliability in IDE server mode depends on it.

---

Minor: RunOptions.EnvironmentVariables mutated via WithEnvironmentVariable

RunOptions is declared as a record with Dictionary<string, string?> EnvironmentVariables { get; init; } = []. WithEnvironmentVariable mutates EnvironmentVariables[key] = value on the existing dictionary rather than returning a new RunOptions with a new dictionary. For a record, this is inconsistent with the With-pattern convention (non-destructive mutation). It works for the current use, but callers who share a RunOptions instance would see side effects. AdditionalArguments has the same pattern in the existing WithArgument — so this is consistent with the existing style — just worth noting if the records are ever used in a more functional style.

---

What's done well

• The root-cause analysis in the PR description is precise and complete.
• DisposeAndClearStaticTrackingAsync uses TryRemove to guard against races with late UntrackObject calls — correct.
• The isForExecution: false skip of argument registration in RegisterTest is the right place and correctly avoids leaking fixture construction into discovery-only requests.
• Dynamic-test and test-variant paths now go through RegisterTestsAsync, closing the registration gap those paths previously had.
• The [After(TestSession)] hook approach for the regression test is clean — inert by default, opt-in via env var.
• Public API snapshots are updated for the new Clear() members.

[claude]

Follow-up Review (updated after second commit)

The second commit (1d0b0a3) cleans up the now-dead ClearStaticTracking() static method that was superseded by DisposeAndClearStaticTrackingAsync. Good housekeeping — the removal was straightforward and correct.

Status of issues raised in the previous review

The substantive concerns from the earlier review remain open. Summarising for visibility:

[Open] Double-disposal of static-property objects
DisposeAndClearStaticTrackingAsync sweeps everything still in s_trackedObjects. Static-property objects are disposed by StaticPropertyHandler.DisposeStaticPropertiesAsync before the sweep, but they are removed from s_trackedObjects via UntrackObject only if their ref count reaches zero. If the count does not drain to zero for a static-property object (e.g. a filter scenario), the sweep will call DisposeAsync on it a second time. The safest fix is to have DisposeStaticPropertiesAsync route through UntrackObject (or ObjectTracker.DisposeAsync) rather than calling _disposer.DisposeAsync directly, so the entry is removed before the sweep runs.

[Open] TestContext.Current clearing removed from BuildTestAsync
The original finally block that set TestContext.Current = null is gone and has no replacement visible in the diff. If anything in BuildTestAsync sets TestContext.Current, the context could leak into the next build iteration on the new path. Worth a quick verification that the clearing still happens somewhere in the new flow.

[Open, lower priority] SessionSharedFixture static counters not reset between runs
Bug6151FilteredDisposalTests uses static int CreatedCount/DisposedCount that accumulate across runs in the same process. The regression test runs in a child process so this is not an issue today, but if the test ever exercises a long-lived server-mode process it would produce Created=2;Disposed=2 on the second iteration.

[Non-blocking] isForExecution boolean flag
Still a minor style point — two named methods (RegisterForExecution/RegisterForDiscovery) would be more self-documenting — but not worth blocking the PR over.

---

The core fix (moving registration to post-filter) is correct and the test coverage is good. The double-disposal concern is the only item I'd want addressed before merging.

[thomhurst]

Addressed the review in d4d6cdf:

Double-disposal of static properties — confirmed real, fixed. DisposeStaticPropertiesAsync disposed directly via Disposer, leaving the +1 entry from TrackStaticProperties in s_trackedObjects, so the new session-end sweep would have disposed those objects a second time. It now goes through a new ObjectTracker.UntrackObjectAsync — decrement to zero, dispose once, entry removed. Verified with static-property suites in both modes.

Instance method on static state — documented: the method needs the injected Disposer (error logging), which only instances carry.

TestContext.Current = null removal — no gap. The only thing that set TestContext.Current in the build path was RegisterTestArgumentsAsync itself (TestArgumentRegistrationService.cs:26); with that call removed from BuildTestAsync, nothing in the build path sets it anymore, so there is nothing to reset. The post-filter RegisterTest call site has never reset it — identical to pre-PR behavior. (InternalDiscoveredTest assignment never touched Current.)

DiscoverTestsFullyStreamingAsync — it has zero callers today; added a warning comment that any future execution use must register tests with isForExecution: true first.

Static counters not reset between runs — fixed: the marker hook now resets CreatedCount/DisposedCount after writing, so a long-lived server process reports per-session counts.

RunOptions.With* mutation — left as-is, consistent with the existing WithArgument pattern as you noted.

[claude]

Final Review — All Outstanding Issues Addressed

All concerns from the two prior review rounds have been resolved in d4d6cdf9b. The PR is ready to merge.

Confirmed addressed

Double-disposal of static-property objects (was the blocking concern)
DisposeStaticPropertiesAsync now calls ObjectTracker.UntrackObjectAsync instead of _disposer.DisposeAsync directly. The new public UntrackObjectAsync method decrements the ref count, disposes at zero, and removes the entry from s_trackedObjects in one operation — exactly the right fix. The session-end sweep will no longer see any entry to dispose a second time. The removal of Disposer from StaticPropertyHandler's constructor is a clean follow-through.

TestContext.Current clearing
Confirmed: only RegisterTestArgumentsAsync set TestContext.Current in the build path, and that call is now removed from BuildTestAsync. No context can leak. The explanation in the PR comment is correct.

Static counters reset between runs
The [After(TestSession)] marker hook now resets CreatedCount/DisposedCount via Interlocked.Exchange after writing the file, so a long-lived server-mode process reports per-session counts correctly.

Streaming discovery path
Defensive comment added to DiscoverTestsFullyStreamingAsync warning that any future execution use must register tests with isForExecution: true first. Good enough given the method has zero callers today.

isForExecution boolean flag
Left as-is and consistent with existing style. Acceptable.

Architecture recap

The fundamental design is sound: registration (shared-object creation + ref counting) moved from build-time to post-filter, with dynamic tests and variants wired into the same post-filter registration path via TestRegistry. The session-end sweep in TestSessionCoordinator handles cancellation/miscount stragglers, and TestDataContainer.Reset() ensures a fresh start for subsequent runs in the same process. The layering is correct and the comments throughout explain the invariants clearly.

Approve.