perf: reduce allocations and lock contention in ObjectTracker

[thomhurst]

Summary

• Eliminate HashSet allocations in FlattenTrackedObjects(): Replaced with IsAlreadyTracked() (O(n) containment check across depth buckets, no allocation) and CountTrackedObjects() (sum counts without materializing a set). These were called twice per test, each allocating a new HashSet<object>.
• Remove intermediate newTrackableObjects HashSet in TrackObjects(): Objects are now tracked directly during iteration instead of being collected first and iterated again.
• Remove global s_disposalLock: Counter.Decrement() uses Interlocked.Decrement which atomically returns the new value, so only one thread can ever observe count == 0 for a given counter. The global lock was unnecessary contention across unrelated objects.
• Parallelize disposal in UntrackObjectsAsync(): Uses Task.WhenAll instead of sequential await for objects that require async disposal, with a fast-path that skips Task allocation for synchronously completed ValueTasks.
• Remove unused using directives (System.Collections.Immutable, System.Diagnostics.CodeAnalysis).

Test plan

• Verify TUnit.Core builds successfully across all target frameworks
• Run existing object tracking tests to confirm no behavioral changes
• Verify disposal ordering is still correct (objects with ref count reaching 0 are disposed exactly once)

[claude]

Code Review: perf: reduce allocations and lock contention in ObjectTracker

Overall this is a solid, well-motivated performance PR. The allocation savings and the analysis of the unnecessary global lock are both correct. A few things worth discussing:

---

✅ Lock removal is correct

The removal of s_disposalLock is valid. Since Interlocked.Decrement atomically returns the new value, exactly one thread can ever observe count == 0 for a given counter. The CAS guarantee from Interlocked makes the global lock unnecessary for this invariant.

---

⚠️ Parallel disposal changes observable semantics

The shift from sequential await UntrackObject(obj) to Task.WhenAll is the biggest semantic change here and deserves explicit acknowledgment.

With sequential disposal, each disposable's DisposeAsync completes before the next starts. With parallel disposal, all async disposables run concurrently. This can cause issues when:

• Two disposables share an underlying resource (e.g., a connection pool, file handle, or lock). Concurrent disposal of both can race on cleanup.
• A disposable's cleanup depends on another object still being alive (ordering assumption).

The depth-bucketed structure of trackedObjects (Dictionary<int, HashSet<object>>) suggests depth ordering was intentional — yet neither the old flat-set approach nor the new parallel approach honours it. If ordering matters at all, it should be enforced at both the tracking and untracking stages.

Suggested improvement: At minimum, document that disposal is now concurrent and unordered. If ordering needs to be preserved in the future, iterate the depth buckets in sorted order and dispose within each bucket in parallel, awaiting each bucket before moving to the next:

---

⚠️ Subtle leak if Counter.Decrement() throws

In UntrackObject:

If RaiseEventSafely inside Decrement() throws AggregateException (which it can), TryRemove is skipped and the object remains in s_trackedObjects forever — a memory leak. This is pre-existing, but the removal of the lock means the exception now propagates differently (previously the lock would release cleanly; now the same outcome applies). Worth a guard:

---

🟡 Redundant early-return in UntrackObjects

Check 1 is fully subsumed by Check 2 (an empty outer dictionary means CountTrackedObjects returns 0 anyway). Check 1 does save one method call in the degenerate case, which is fine, but the two-check pattern is mildly confusing without a comment. A single CountTrackedObjects check with an explanatory comment would be clearer.

---

✅ IsAlreadyTracked / CountTrackedObjects trade-off is reasonable

Replacing the materialized HashSet with linear scans across depth buckets is the right call. In practice, depth counts are small (typically 1–3), so the O(d) factor is negligible and the allocation savings are real. The naming is clear and the XML docs explain the reasoning.

---

✅ Removal of newTrackableObjects intermediate set

Calling TrackObject directly inside the lock — rather than collecting into an intermediate HashSet and iterating again — is correct and cleaner. TrackObject only touches s_trackedObjects (the global ConcurrentDictionary), not the locked kvp.Value, so there's no deadlock risk.

---

Summary: The allocation reduction goals are well-executed. The lock removal analysis is sound. The main concern is whether parallel disposal is safe for all disposables in the test fixture graph — if it is, that should be documented; if it isn't, sequential-per-depth-bucket disposal would be safer.

[claude]

Code Review: perf: reduce allocations and lock contention in ObjectTracker

Overall this is a solid, well-motivated performance PR. The allocation savings and the analysis of the unnecessary global lock are both correct. A few things worth discussing.

---

Lock removal is correct

The removal of s_disposalLock is valid. Since Interlocked.Decrement atomically returns the new value, exactly one thread can ever observe count == 0 for a given counter. The lock was unnecessary for this invariant.

---

Parallel disposal changes observable semantics

The shift from sequential await UntrackObject(obj) to Task.WhenAll is the biggest semantic change here and deserves explicit acknowledgment.

With sequential disposal, each disposable's DisposeAsync completes before the next starts. With parallel disposal, all async disposables run concurrently. This can cause problems when:

• Two disposables share an underlying resource (connection pool, file handle, lock). Concurrent disposal of both can race on cleanup.
• A disposable's cleanup depends on another object still being alive.

The depth-bucketed structure of trackedObjects (Dictionary<int, HashSet<object>>) suggests depth ordering was intentional. Neither the old flat-set approach nor the new parallel approach honours it — but the old approach was at least sequential. If ordering ever needs to be enforced, iterating depth buckets in sorted order (disposing each bucket in parallel, but awaiting before moving to the next) would be the right architecture.

At minimum, a comment documenting that disposal is now concurrent and unordered would help future maintainers understand the intent.

---

Subtle pre-existing leak if Counter.Decrement() throws

In UntrackObject, if RaiseEventSafely inside Decrement() throws AggregateException (which it can when event handlers fail), TryRemove is never reached and the object stays in s_trackedObjects permanently. This is pre-existing, not introduced by this PR, but worth a guard:

int count;
try { count = counter.Decrement(); }
catch { s_trackedObjects.TryRemove(obj, out _); throw; }

---

Minor: redundant early-return in UntrackObjects

The trackedObjects.Count == 0 check is fully subsumed by CountTrackedObjects(trackedObjects) == 0. The first saves one loop iteration in the degenerate case, which is fine, but without a comment it reads as accidental duplication.

---

Positives

• Removing newTrackableObjects and tracking directly inside the lock is correct — TrackObject only touches the global s_trackedObjects, so no deadlock risk.
• IsAlreadyTracked / CountTrackedObjects O(d) scan is the right trade-off for typical small depth counts (1–3). The naming and docs are clear.
• disposalTasks ??= [] lazy allocation is a nice touch.

---

Summary: Good allocation wins and a sound lock-removal argument. The main open question is whether parallel disposal is safe for all disposables in a test fixture graph. If yes, document it; if not, ordered-per-depth-bucket parallel disposal is the safer design.

[claude]

Follow-up Review

Previous review comments (2026-03-22) raised four concerns. Here's where each stands in the current diff:

---

✅ Lock removal — correct, no change needed

Confirmed valid. The Interlocked.Decrement atomicity guarantee means exactly one thread can observe count == 0. The global lock was unnecessary.

---

✅ Subtle leak if Counter.Decrement() throws — already addressed

The try/catch guard is already present in the current diff:

int count;
try { count = counter.Decrement(); }
catch { s_trackedObjects.TryRemove(obj, out _); throw; }

Good defensive coding.

---

✅ Redundant early-return — addressed

UntrackObjects now uses a single CountTrackedObjects == 0 check rather than a duplicated outer Count == 0 guard. Cleaner.

---

⚠️ Parallel disposal ordering — still unaddressed

The previous review flagged this as the main open concern. The shift from sequential await UntrackObject(obj) to Task.WhenAll is still in the diff without any comment explaining that disposal is now concurrent and unordered.

The depth-bucketed structure (Dictionary<int, HashSet<object>>) implies intentional ordering. The old flat-set approach lost the ordering too, but at least iterated sequentially within whatever order the flat-set happened to produce. The new approach now runs all async disposals concurrently, which can cause subtle issues if:

• Two disposables share an underlying resource (database connection, file handle)
• A disposable's cleanup depends on another object still being alive

Minimum ask: Add a comment near the Task.WhenAll call documenting that disposal is concurrent and unordered, and that callers must not rely on ordering between depth buckets. This makes the intent explicit rather than leaving it as an accidental property.

Better approach (if ordering ever matters): Iterate depth keys in sorted order, disposing each bucket in parallel but awaiting before moving to the next:

foreach (var depth in trackedObjects.Keys.OrderByDescending(k => k))
{
    List<Task>? bucketTasks = null;
    foreach (var obj in trackedObjects[depth])
    {
        var task = UntrackObject(obj);
        if (!task.IsCompletedSuccessfully)
        {
            bucketTasks ??= [];
            bucketTasks.Add(task.AsTask());
        }
    }
    if (bucketTasks is { Count: > 0 })
        await Task.WhenAll(bucketTasks).ConfigureAwait(false);
}

This preserves the parallelism win within a depth level while respecting cross-depth ordering.

---

Summary

Three of four prior concerns are resolved. The parallel disposal concern remains the one thing blocking a clean approval — at minimum a clarifying comment, ideally depth-ordered disposal if the structure is semantically meaningful.

[claude]

Follow-up Review (3rd pass)

Tracking the four concerns from previous reviews:

---

✅ Lock removal — correct

Interlocked.Decrement atomicity ensures exactly one thread observes count == 0. Lock removal is valid.

---

✅ Leak if Counter.Decrement() throws — addressed

The try/catch guard is present and correct:

int count;
try { count = counter.Decrement(); }
catch { s_trackedObjects.TryRemove(obj, out _); throw; }

---

✅ Depth ordering — now addressed

The previous review's core complaint is now fixed. The new code iterates depth buckets in descending order and awaits each bucket before moving to the next:

foreach (var depth in trackedObjects.Keys.OrderByDescending(k => k))
{
    // ...
    await Task.WhenAll(disposalTasks).ConfigureAwait(false);
}

Objects at deeper levels (inner dependencies) are disposed before shallower ones, and parallelism is scoped within a single depth level. This is the recommended architecture from the previous review.

---

⚠️ Bug: AggregateException catch is dead code — exceptions are silently dropped

This is a new issue introduced by the Task.WhenAll parallelisation.

When you await Task.WhenAll(...) and multiple tasks throw, the C# awaiter unwraps the AggregateException and only rethrows the first inner exception. The catch (AggregateException ae) branch will never fire in normal circumstances (it would only match if a disposal method itself explicitly throws an AggregateException, which is rare). All other disposal exceptions are silently swallowed:

// Bug: if 3 tasks throw, only the first exception is caught
catch (AggregateException ae)          // dead code path via await
{
    foreach (var e in ae.InnerExceptions) // never reached
        cleanupExceptions.Add(e);
}
catch (Exception e)
{
    cleanupExceptions.Add(e);             // only captures the 1st failure
}

Fix: Capture the Task before awaiting, then inspect its Exception.InnerExceptions to collect all failures:

if (disposalTasks is { Count: > 0 })
{
    var whenAllTask = Task.WhenAll(disposalTasks);
    try
    {
        await whenAllTask.ConfigureAwait(false);
    }
    catch
    {
        // Task.WhenAll wraps all failures in AggregateException on the Task itself
        // but await only rethrows the first — so read InnerExceptions directly
        foreach (var e in whenAllTask.Exception?.InnerExceptions ?? [])
        {
            cleanupExceptions.Add(e);
        }
    }
}

Why this matters: The whole point of collecting cleanupExceptions is to surface all disposal failures after a test run. Silently dropping all-but-first disposal error hides real issues in the scenario where multiple objects in the same depth bucket fail to dispose.

---

Summary

The PR is in very good shape. The depth-ordering concern from prior reviews is now properly addressed. The only remaining issue is the Task.WhenAll exception collection bug above, which is a straightforward fix.

[claude]

Follow-up Review (4th pass → final)

Tracking all concerns raised in previous reviews against the current diff (commit 745f0d1):

---

✅ Lock removal — valid, no change needed

Interlocked.Decrement atomicity ensures exactly one thread observes count == 0. Lock removal is correct.

---

✅ Leak if Counter.Decrement() throws — addressed

The guard is in place and correct:

int count;
try { count = counter.Decrement(); }
catch { s_trackedObjects.TryRemove(obj, out _); throw; }

---

✅ Depth ordering — addressed

Depth buckets are now iterated in descending order and each bucket is fully awaited before moving to the next — parallelism within a level, ordering preserved across levels. This was the recommended architecture from the prior review.

---

✅ AggregateException dead-code bug — addressed

The 4th-pass review identified that catch (AggregateException ae) was dead code because await unwraps to the first inner exception only. The current diff correctly captures the Task before awaiting and reads InnerExceptions directly:

var whenAllTask = Task.WhenAll(disposalTasks);
try
{
    await whenAllTask.ConfigureAwait(false);
}
catch
{
    foreach (var e in whenAllTask.Exception!.InnerExceptions)
        cleanupExceptions.Add(e);
}

All disposal failures in a depth bucket are now surfaced. Good fix.

---

Minor observation (non-blocking)

The outer catch (Exception e) wrapping the UntrackObject(obj) call inside the bucket loop is dead code for exceptions from within UntrackObject, because it is an async method — all its exceptions are folded into the returned ValueTask, never thrown synchronously. Those faulted tasks are correctly handled via the disposalTasks + whenAllTask path anyway, so there is no correctness issue — it's just a misleading catch that could be removed or replaced with a comment explaining why exceptions come via the task.

---

Summary

All four concerns from prior reviews are resolved. The allocation wins are real and correct, the lock removal analysis is sound, depth ordering is preserved, and all disposal exceptions are now properly collected. This is ready to merge.

[claude]

Code Review — perf: reduce allocations and lock contention in ObjectTracker

This is a follow-up review tracking the four concerns raised in previous rounds. All have been addressed.

---

✅ All Previous Concerns Resolved

Concern	Status
Lock removal correctness (s_disposalLock)	✅ Valid — Interlocked.Decrement atomicity guarantees exactly one thread observes count == 0
Leak if Counter.Decrement() throws	✅ Fixed — try/catch guard calls TryRemove before rethrowing
Depth ordering preserved during disposal	✅ Fixed — OrderByDescending(k => k) with per-bucket await
AggregateException dead-code / silent exception drop	✅ Fixed — captures whenAllTask before await, reads InnerExceptions directly to collect all failures

---

Minor Non-Blocking Observation

The outer catch (Exception e) wrapping UntrackObject(obj) in the bucket loop is technically dead code for async exceptions:

foreach (var obj in bucket)
{
    try
    {
        var task = UntrackObject(obj);   // async — exceptions fold into the ValueTask
        if (!task.IsCompletedSuccessfully)
        {
            disposalTasks ??= [];
            disposalTasks.Add(task.AsTask());
        }
    }
    catch (Exception e)    // ← only catches synchronous exceptions from UntrackObject
    {
        cleanupExceptions.Add(e);
    }
}

Since UntrackObject is an async method, all its exceptions (including those from IAsyncDisposable.DisposeAsync()) are captured into the returned ValueTask, not thrown synchronously. Async faults are already correctly handled by the whenAllTask path below. The catch here will only ever fire for a true synchronous throw at the very start of UntrackObject (before the first await) — which isn't the common case. This isn't a correctness issue since the happy path is covered, but it could be annotated with a comment to clarify intent, or removed if it's known that UntrackObject never throws synchronously.

Not a blocker — the disposal failure collection is sound end-to-end.

---

Overall Assessment

The performance goals are achieved cleanly:

• Zero allocations in the fast-path of FlattenTrackedObjects (now CountTrackedObjects)
• Lock contention eliminated with a correct lock-free analysis
• Parallel disposal scoped within each depth bucket, with depth ordering preserved between buckets
• All disposal exceptions surfaced correctly via whenAllTask.Exception.InnerExceptions

The code is well-structured and the changes are well-reasoned. Ready to merge.