Fix DisposeAsync() ordering for nested property injection

[Copilot]

• Investigate the issue: DisposeAsync ordering is wrong for nested property injection
• Identify root cause: ObjectTracker.UntrackObjectsAsync iterates deepest-to-shallowest (same as initialization), should be shallowest-to-deepest (reverse of initialization)
• Fix the disposal ordering in ObjectTracker.UntrackObjectsAsync to iterate ascending (shallowest first)
• Add a regression test for nested dispose ordering
• Run existing tests to verify no regressions
• Run code review and CodeQL

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[thomhurst]

@copilot continue

[claude]

Code Review: Fix DisposeAsync() ordering for nested property injection

Summary

This PR fixes a real bug: ObjectTracker.UntrackObjectsAsync was iterating depth buckets from deepest-to-shallowest (reverse), but because depth is assigned such that shallow objects get lower depth numbers (depth 0 = root, depth 1+ = nested dependencies), disposal was actually happening in the wrong order — deep objects (dependencies) were being disposed before shallow ones (dependents). The fix correctly changes the iteration to ascending order.

The core fix is minimal and correct. The regression test is a welcome addition. A few observations worth raising:

---

Issue 1: Depth Semantics Are Counterintuitive — and the Fix Inverts Them

The comment in the fix says "iterate in ascending order (shallowest depth first)", and that's true. But looking at ObjectGraphDiscoverer, depth 0 is the test class's directly injected properties (e.g. AppServiceFixture), and deeper numbers represent their dependencies (e.g. ContextFactoryFixture ends up at depth 2). So "ascending order" means "dependents first, dependencies last", which is the correct disposal order.

The issue is that the naming is backwards from how most people think about depth: depth 0 is the "shallowest" injected object (a leaf from the test's perspective), but it depends on depth-1 objects. This creates a mental model mismatch where "deeper depth = more fundamental dependency" is the opposite of what "deeper" usually implies.

This is a pre-existing design issue, not introduced by this PR — but if this area is touched again in the future, consider renaming to something like injectionLevel or initializationOrder to make the intent clearer. The old comment "iterate by index in reverse for descending depth" was accurate (it did iterate reverse), but the effect was the bug: it was disposing dependencies first.

Suggestion: Add a brief explanatory note to TestContext.TrackedObjects or ObjectGraphDiscoverer.CollectRootObjects explaining the depth convention (0 = test class's direct deps, higher = deeper transitive deps that are initialized first).

---

Issue 2: Regression Test Uses [After(TestSession)] with a Verification Guard That May Miss Failures

[After(TestSession)]
public static async Task VerifyDisposalOrder(TestSessionContext context)
{
    var initOrder = NestedDisposalOrderTracker.GetInitOrder();
    var disposeOrder = NestedDisposalOrderTracker.GetDisposeOrder();

    // Guard: skip assertions if this test class was not part of the test run
    if (initOrder.Count == 0)
    {
        return;
    }
    ...
}

The [After(TestSession)] hook runs for the entire test session. The guard if (initOrder.Count == 0) return; silently skips verification if the fixtures were never initialized — which could happen if the test was filtered out, or if the test is run in isolation. In those cases, the regression guard is a no-op, giving false confidence.

Suggestion: Consider structuring the verification as an [After(Class)] hook on NestedDisposalOrderTests instead of [After(TestSession)]. That way the verification is tightly scoped to the test class, runs only when the class ran, and doesn't require the silent-skip guard. Alternatively, if [After(TestSession)] is required for timing reasons (disposal happens after the session ends), document why explicitly.

---

Issue 3: NestedDisposalOrderTracker is a Static Global — Fragile in Parallel Runs

NestedDisposalOrderTracker uses static List<string> fields guarded by a Lock. The [Before(Class)] hook calls Reset() before each test class run. This pattern is fragile:

• If tests from NestedDisposalOrderTests run in parallel with other test classes that happen to init/dispose the same shared fixtures, the recorded orders could interleave.
• The [NotInParallel] attribute helps, but only prevents parallelism between tests within this class. Other classes sharing PerTestSession fixtures could still interfere.
• More importantly, the static tracker is reset in [Before(Class)] but the verification happens [After(TestSession)] — so if anything resets it again between class teardown and session end, the verification would silently pass via the guard.

Since SharedType.PerTestSession is used, the fixtures are shared across the session anyway, so the init/dispose ordering applies to the whole session. The test design is probably fine in practice, but the static global state pattern is fragile and could cause false passes or test pollution if the structure changes.

Suggestion: Consider whether the tracking could be done via the fixture constructors/disposers themselves (e.g. via a shared InjectedOrderRecorder fixture), rather than a static tracker. That would be more aligned with how TUnit fixtures are meant to be used.

---

Minor: Unused Variable Removal is Clean

The removal of the now-unused keys variable (var keys = trackedObjects.Keys;) is correct — values is accessed directly by index, so keys is no longer needed. Good cleanup.

---

Overall Assessment

The core fix is correct and addresses a real disposal ordering bug. The test is valuable. The concerns above are about test robustness and a pre-existing naming/documentation issue — they shouldn't block merging, but Issue 2 in particular (the silent-skip guard in the session-level hook) could mean the regression test doesn't actually catch regressions in certain execution contexts.