[thomhurst]

Summary

When a nested property getter throws while ObjectTracker.TrackObjects discovers a test's object graph, the rollback added in #7191 removed everything discovery had added, including the test's root argument objects. Those roots were never counted, so neither test completion nor the session-end sweep disposed them.

• IObjectGraphDiscoverer.TrackRootObjects (internal) adds only the test's roots at depth 0: class and method arguments, direct injected property values, and unreceived data-row objects. It does not read any property of them. Unreceived row objects it adds are marked disposal-only, as in normal discovery (First class-data fetch per class row creates an untracked instance that is never disposed (SharedType.None) #7157).
• ObjectTracker.TrackObjects still rolls back what the failed discovery added, then tracks and counts the roots. Nested objects stay dropped, and shared roots get one reference for this test, which completing the test releases. The rollback guarantee from fix(engine): dispose data-source objects of skipped tests and rows #7191 still holds: the test releases only references it took.
• This covers both paths in the issue. A test that fails during registration never initializes its tracked objects, and its roots are disposed in TestCoordinator's finally. A skipped test's roots are now disposed too. I updated the comment in TestFilterService to match.

Tests

• ObjectTrackerRollbackTests.Failed_Discovery_Still_Tracks_And_Disposes_Root_Objects: the root is disposed, the nested object is dropped, and a shared root survives until its other holder releases it.
• Bugs/_7213 plus Issue7213Tests: a ClassDataSource root whose IAsyncInitializer getter throws is disposed, both for a test that fails during registration and for a skipped test. The observer test fails without the fix and passes with it.

Local validation (net10.0):

• TUnit.UnitTests: all 669 tests run, 665 passed and 4 skipped.
• TUnit.Engine.Tests Issue7213Tests and Issue7156Tests, reflection mode: pass. The AOT engine mode runs only in CI.
• TUnit.TestProject with --treenode-filter "/*/TUnit.TestProject.Bugs._7213/*/*" in source-generated mode: the observer passes, and the registration failure is expected.

No public API change: the interface is internal.

Closes #7213

Summary by CodeRabbit

• Bug Fixes
  • Root objects are now disposed even when object discovery fails, including when property access throws. Nested objects discovered before the failure are rolled back, and the original discovery error is preserved.

[thomhurst]

@codex review

[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: cc90fc69-73e5-4992-a159-bbc70f263a70

Commits

Reviewing files that changed from the base of the PR and between 5329979 and 31a95f5.

Files selected for processing: 1

• tests/TUnit.TestProject/Bugs/_7213/ThrowingGetterRootDisposalTests.cs

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

Walkthrough

When object-graph discovery fails, ObjectTracker attempts to track root objects without traversing their properties. It then rethrows the original discovery exception. Regression tests cover disposal for failed and skipped tests.

Changes: Root disposal after discovery failure

Layer / File(s)	Summary
Root-only discovery contract and implementation src/TUnit.Core/Interfaces/IObjectGraphDiscoverer.cs, src/TUnit.Core/Discovery/ObjectGraphDiscoverer.cs	The discoverer exposes root-only tracking. It tracks depth-zero objects without traversing their properties.
Failure fallback and root tracking integration src/TUnit.Core/Tracking/ObjectTracker.cs, src/TUnit.Core/Tracking/TrackableObjectGraphProvider.cs, src/TUnit.Engine/Services/TestFilterService.cs	After full discovery fails, ObjectTracker removes newly discovered objects, attempts to track roots, and rethrows the original exception. The provider delegates root tracking to the discoverer. Comments describe root disposal for skipped tests.
Disposal and rollback regression coverage tests/TUnit.UnitTests/ObjectTrackerRollbackTests.cs, tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs, tests/TUnit.TestProject/Bugs/_7213/ThrowingGetterRootDisposalTests.cs, tests/TUnit.Engine.Tests/Issue7213Tests.cs	Tests cover fallback rollback, root disposal, throwing property getters, and the resulting failed, passed, and not-executed test results.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: PATCH

Merge Risk: Minimal · up to 31a95

The reviewed changes have no unresolved merge-blocking concerns in the supplied evidence.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.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. Documentat…	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: preserving and disposing data-source root objects when object-graph tracking throws.
Linked Issues check		Issue #7213 requires disposal of root objects when nested discovery throws, for registration failures and skipped tests. ObjectTracker.TrackObjects now rolls back discovery additions, tracks roots w…
Out of Scope Changes check		The changes stay within issue #7213. Production changes implement root-only fallback tracking, rollback, exception preservation, and the related comment update. The added tests verify the required dis…

Full details: Linked Issues check

Explanation

Issue #7213 requires disposal of root objects when nested discovery throws, for registration failures and skipped tests. ObjectTracker.TrackObjects now rolls back discovery additions, tracks roots without property traversal, and preserves the original exception. The implementation retains root reference counts and keeps unreceived row objects disposal-only. ObjectTrackerRollbackTests covers rollback, root disposal, nested-object removal, and shared-root counts. Issue7213Tests and the _7213 test project cover registration failure and skipped-test disposal.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #7213. Production changes implement root-only fallback tracking, rollback, exception preservation, and the related comment update. The added tests verify the required disposal and reference-count behavior. No unrelated behavior or public API change is demonstrated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.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 5 functions across 9 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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A root stays tracked when getters fail,
Its nested branches leave the trail.
The original error still returns,
While disposal waits its turn.
A skipped test has roots in view,
And shared counts remain true.}

[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-11-01T21:18:05.221746Z	31a95f5	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.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: c485852520

ℹ️ 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

The fix looks correct and well scoped. After a failed discovery, ObjectTracker.TrackObjects rolls back to the snapshot (so the #7191 guarantee holds). It then tracks only the depth-0 roots and skips anything already in the snapshot, so no reference is double-counted. TrackRootObjects reads no properties, so it can't hit the same throwing getter. Unreceived row objects keep their disposal-only marking, as in #7157. The unit test covers the shared-root case, and the engine test covers both the registration-failure and skipped paths.

Minor suggestions (none blocking):

• Duplicated unreceived-row logic. ObjectGraphDiscoverer.TrackRootObjects repeats the unreceived-row handling from TrackUnreceivedDataRowObjects. Over time the two could drift, for example if disposal-only rules change. I'd consider having DiscoverAndTrackObjects and TrackRootObjects share one helper, with the nested traversal passed as a no-op or a real callback.
• Stale disposal-only entries after rollback. RemoveObjectsNotIn doesn't touch DisposalOnlyTrackedObjects. If the failed discovery had already added an unreceived row object to that set, the rollback drops it from the tracked set but leaves the disposal-only entry. This is harmless today, because TrackRootObjects re-adds the object. A one-line comment would help future readers.
• No failure handling around TrackRootObjects. If it threw inside the catch block, the original discovery exception would be masked. It does no user-code reads, so this is unlikely, but wrapping it so the original exception always propagates would be cheap.
• Interface growth. TrackRootObjects is internal and needs fake implementations in two test files. That's fine, but a default interface implementation would cut churn if more fakes appear.

Nothing blocks merge from my side.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

Keeps root objects tracked for disposal when nested object discovery throws.

• TUnit tracks test roots so cleanup can dispose them when nested discovery throws.

_{Reviews (4) · Last reviewed commit: "test(7213): tag the disposal observer as..." · Reviewed by Greptile}

[github-actions]

Review: PR 7342 (root disposal after a failed discovery)

I read the diff and the surrounding ObjectTracker and ObjectGraphDiscoverer code but did not run the tests. I found no blocking issues. The fix is small and sits where the failure happens. It keeps the #7191 rollback guarantee: the test only releases references it took. Roots are counted only after the rollback, and the alreadyTrackedSnapshot check stops a shared root from being counted twice.

Two suggestions:

1. The catch block could mask the original exception. TrackRootObjects(...) runs inside catch before throw;. If it throws (a user Equals or GetHashCode on a hash-set add, or a throwing argument enumerator), that exception replaces the getter exception and TestFilterService sees the wrong cause. Wrap it in a best-effort try { ... } catch { /* keep original */ }, so the original failure always surfaces.

2. Row-object handling is duplicated. TrackRootObjects copies the unreceived-row logic from DiscoverAndTrackObjects/TrackUnreceivedDataRowObjects, including the disposal-only marking from First class-data fetch per class row creates an untracked instance that is never disposed (SharedType.None) #7157. Both copies must stay in sync if that rule changes. It would help to extract a shared helper, or at least add a comment in each place pointing at the other. The early-return guard at the top also repeats the one in DiscoverAndTrackObjects.

Minor: TrackableObjectGraphProvider.GetTrackableRootObjects is a pure pass-through. That is fine, since it matches the existing provider pattern.

The tests cover both the unit-level behaviour (shared root, nested object dropped) and the engine-level cases (failed registration and skipped). The observer test, which uses DependsOn with ProceedOnFailure, is a good way to check disposal end to end.

Overall this looks good, with suggestion 1 worth taking before merge.

[thomhurst]

Review dispositions for 5329979 (replies to both Claude reviews):

• Fallback could mask the discovery exception. Fixed in 5329979. ObjectTracker.TrackRootObjects now catches a throw from the root-only fallback, rolls the tracked set back to the snapshot again, and returns, so the caller's throw; rethrows the original exception. Nothing is counted until the fallback returns, so the rollback leaves the references balanced. New test: Failing_Root_Fallback_Keeps_The_Discovery_Exception_And_Rolls_Back.
• Duplicated unreceived-row logic and early-return guard. Fixed in 5329979. DiscoverAndTrackObjects and TrackRootObjects now share one private TrackObjectsCore(testContext, discoverNested, ct). The fallback passes a no-op nested callback, so both use the same guard and the same TrackUnreceivedDataRowObjects disposal-only rule (First class-data fetch per class row creates an untracked instance that is never disposed (SharedType.None) #7157).
• Stale disposal-only entries after rollback. No change. As you noted, it is harmless: the fallback goes through the same row tracking, and TrackUnreceivedDataRowObjects reassigns DisposalOnlyTrackedObjects.
• Default interface implementation for the internal interface. No change. There are only two internal fakes, and a default method would hide a missing implementation from future fakes.
• GetTrackableRootObjects pass-through. No change. It matches the existing provider pattern, as you noted.

Validation (net10.0): TUnit.UnitTests ran 671 tests: 667 passed and 4 skipped. TUnit.TestProject with --treenode-filter "/*/TUnit.TestProject.Bugs._7213/*/*" behaves the same in source-generated and reflection modes: the observer passes, the skipped test is skipped, and only the intended registration failure fails.

[thomhurst]

@codex review

[github-actions]

Review

The PR fixes a real leak. The rollback from #7191 dropped the test's root objects along with the nested ones, so nothing ever disposed them. Re-adding only the roots and counting them is a sound, minimal fix.

What I checked

• Reference balance: the rollback removes objects added by this call, and TrackRootObjects counts only roots that weren't in the pre-call snapshot. So the test releases only references it took. That keeps the fix(engine): dispose data-source objects of skipped tests and rows #7191 guarantee and the shared-root case.
• Code reuse: TrackObjectsCore shares the root-collection and unreceived-row logic through a discoverNested flag, so the two paths can't drift apart.
• Fallback failure: a failing fallback rolls back again and the original discovery exception still propagates (Failing_Root_Fallback_Keeps_The_Discovery_Exception_And_Rolls_Back). Good defensive behavior.
• Tests: both the unit tests and the end-to-end _7213 tests cover the registration-failure and skipped paths.
• Public API: the interface is internal, so no API snapshot change.

Minor suggestions (non-blocking)

• TrackRootObjects on the discoverer ignores cancellation (CancellationToken.None). That is fine because it does no user-code reads, but it relies on CollectRootObjects never reading getters. A short comment, or a unit test asserting that no property getter is touched during TrackRootObjects, would guard that invariant against future changes.
• The TrackRoots default in FakeDiscoverer is duplicated in two test files. Fine for now, but a shared test helper would reduce drift if the interface grows again.

LGTM.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 53299796d5

ℹ️ 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".

[thomhurst]

Dispositions for the review of 5329979 (both suggestions are non-blocking, so this head stays unchanged):

• TrackRootObjects relies on CollectRootObjects never reading getters. No change in this PR. The fallback reads only values the engine already holds (arguments, injected property values, and row objects), never a property getter. If a future change makes the fallback throw, ObjectTracker catches it, rolls back, and the original exception still propagates (Failing_Root_Fallback_Keeps_The_Discovery_Exception_And_Rolls_Back). So a regression would mean a missed disposal, not a masked failure.
• FakeDiscoverer.TrackRoots duplicated in two test files. No change. These are two small internal test fakes. A shared helper is worth adding only if a third one appears.

Claude and Codex both approve this head with no blocking findings, so I'm marking the PR ready.

[github-actions]

Review

The change looks correct and well scoped. It fixes the leak in #7213 without breaking the #7191 rollback guarantee.

What I checked

• TrackObjects rolls back what the failed discovery added, then tracks and counts only the depth-0 roots that weren't already tracked. A re-registered test therefore keeps its old references and takes no new ones for objects it already held.
• The fallback is wrapped in its own try/catch. It rolls back again and the original exception is rethrown. Failing_Root_Fallback_Keeps_The_Discovery_Exception_And_Rolls_Back covers that path.
• TrackRootObjects reuses TrackObjectsCore with a no-op nested callback, so root collection and the disposal-only marking for unreceived rows (First class-data fetch per class row creates an untracked instance that is never disposed (SharedType.None) #7157) stay in one place. There's no duplicated traversal logic.
• IObjectGraphDiscoverer is internal, so there's no public API change.
• The tests cover the root cases: the shared-root refcount, the nested object being dropped, and the failed and skipped engine paths.

Minor suggestions (non-blocking)

1. TrackObjectsCore is now mode-switched by a bool and a static _ => {} lambda. It works, but passing null for the callback and null-checking it in CollectRootObjects and ProcessRootCollection would be a bit leaner. It would also avoid relying on a cached static delegate being picked up.
2. In the unreceived-rows step, receivedObjects is built from everything in visitedObjects. In the fallback, that includes objects kept from an earlier registration. This is probably harmless, since those were already classified. Add a short comment saying so, or a test, to pin the behavior.
3. TrackRootObjects hard-codes CancellationToken.None. Since the fallback is deliberately best-effort and doesn't read properties, that's fine. The comment already explains why.

No blocking issues. Nice work.

[thomhurst]

CI on 5329979 failed in ExpectedStateTests.Pass (modularpipeline and AOT, every OS): ThrowingGetterRootDisposalObserverTests was tagged EngineTest=Pass, but its DependsOn pulls FailsDuringRegistration into that run. Fixed in 31a95f5: the observer is now tagged Failure, following the _6554 and _7123 pattern. Local check (net10.0, source-gen and reflection): the [EngineTest=Pass] filter on Bugs._7213 runs 1 test, which is skipped, with 0 failures. The [EngineTest=Failure] filter and the full namespace both give 3 total, 1 passed, 1 failed, 1 skipped, which matches Issue7213Tests.

Dispositions for the non-blocking suggestions in the Claude review of 5329979:

1. Use a null callback instead of the static _ => {} lambda. Not changed. A static lambda is cached once by the compiler, and the change would add null checks to the root-collection hot path for no behavior gain.
2. receivedObjects includes objects from an earlier registration in the fallback. Not changed. Those objects were already classified when they were first tracked, so they are correctly treated as received and are not re-marked disposal-only.
3. CancellationToken.None in TrackRootObjects. Agreed, this is intended. The fallback is best-effort and reads no properties, and the existing comment explains this.

[github-actions]

Review

The fix looks correct and well scoped. I reviewed the diff only and didn't run the tests.

• Roll back the failed nested discovery, then track and count only the depth-0 roots. That keeps the fix(engine): dispose data-source objects of skipped tests and rows #7191 guarantee that a test only releases references it took.
• TrackRootObjects reuses TrackObjectsCore with a no-op root callback, so root collection isn't duplicated.
• The root fallback is best-effort. If it throws, it rolls back and the original discovery exception is rethrown. Failing_Root_Fallback_Keeps_The_Discovery_Exception_And_Rolls_Back covers that.
• The shared-root case is tested: a root held by another test survives until that holder releases it.
• The engine test covers both paths from the issue, failure during registration and a skipped test. The DependsOn observer pattern is a bit indirect but is explained in a comment.

Minor, non-blocking: TrackRootObjects on the discoverer takes no CancellationToken and passes CancellationToken.None. That's fine while root collection doesn't read user properties. If it ever starts to, thread the token through.

No blocking issues. LGTM.