fix: Refactor disposal object tracking

[thomhurst]

Fixes #3219

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3227: "fix: add OnTestFinalized event for proper resource management after retries"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

Review Summary

This is an excellent fix that addresses a critical resource management issue with retries. The implementation is well-designed and follows TUnit's architectural principles.

🟢 Strengths

Architecture & Design

• Clean separation of concerns: The OnTestFinalized event is properly scoped as internal for framework use
• Correct event placement: Firing in the finally block ensures it always executes regardless of test outcome
• Lifecycle clarity: Clear distinction between OnTestEnd (per attempt) vs OnTestFinalized (after all retries)

Code Quality

• Follows coding standards: Adheres to TUnit's modern C# conventions
• Good documentation: Clear XML comments explaining the event's purpose
• Exception handling: Proper try-catch around event invocation with logging in TestCoordinator.cs:108-119
• Ordering support: Uses InvocationList.OrderBy(x => x.Order) for deterministic execution

Test Coverage

• Comprehensive test scenarios: Both basic reproduction test and comprehensive verification
• Real-world simulation: Tests with IAsyncInitializer and IAsyncDisposable classes
• Dependency verification: Uses DependsOn to verify disposal timing
• Console output: Helpful debugging information for manual verification

🟡 Minor Considerations

Performance

• Minimal overhead: The event invocation adds negligible performance cost
• Memory efficiency: No additional allocations in hot paths
• Event ordering: The OrderBy operation on invocation list has minimal cost for typical use cases

Thread Safety

• Event registration: The implementation appears thread-safe as events are typically registered during test setup
• ObjectTracker integration: The reference counting logic is already thread-safe with concurrent collections

🔍 Code-Specific Analysis

TestContextEvents.cs:16-20

/// <summary>
/// Internal framework event that fires after all retry attempts are complete.
/// Used for framework resource management and reference counting.
/// </summary>
internal AsyncEvent<TestContext>? OnTestFinalized { get; set; }

• ✅ Excellent documentation explaining the specific use case
• ✅ Proper access modifier (internal) prevents external misuse
• ✅ Consistent typing with other events in the class

ObjectTracker.cs:53

events.OnTestFinalized += async (_, _) =>

• ✅ Correct event usage - now uses OnTestFinalized instead of previous event
• ✅ Proper async handling with ConfigureAwait(false)
• ✅ Reference counting logic remains unchanged and robust

TestCoordinator.cs:106-119

if (test.Context.Events.OnTestFinalized != null)
{
    try
    {
        foreach (var invocation in test.Context.Events.OnTestFinalized.InvocationList.OrderBy(x => x.Order))
        {
            await invocation.InvokeAsync(test.Context, test.Context);
        }
    }
    catch (Exception ex)
    {
        _logger.LogError($"Error during test finalization for {test.TestId}: {ex}");
    }
}

• ✅ Proper null checking before event invocation
• ✅ Exception isolation prevents finalization errors from affecting test results
• ✅ Deterministic ordering with OrderBy(x => x.Order)
• ✅ Appropriate logging for debugging finalization issues

🟢 Security Assessment

• No security concerns: The changes are internal framework modifications
• Access control: Proper use of internal modifier restricts external access
• Exception handling: Errors are logged but don't expose sensitive information

🟢 Compatibility

• Behavioral parity: The fix should work identically in both source-generated and reflection modes
• No breaking changes: All changes are additive or internal
• AOT-friendly: No new reflection usage introduced

🎯 Recommendations

Consider for Future Enhancement (not blocking)

1. Performance metric: Consider adding a simple counter for OnTestFinalized invocations for debugging
2. Documentation: Consider adding a comment in TestCoordinator explaining why the event fires in finally

Testing Suggestions (already well covered)

• ✅ Test with multiple retry attempts
• ✅ Test with async disposable resources
• ✅ Test timing of disposal vs retry attempts
• ✅ Test with shared resources across test session

🚀 Verdict

APPROVED - This is a high-quality fix that:

• Solves the reported issue completely
• Follows TUnit's architectural principles
• Includes excellent test coverage
• Has minimal performance impact
• Maintains backward compatibility

The implementation correctly ensures that resource disposal only happens after all retry attempts are complete, which was the core issue reported in #3219.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3227: "fix: add OnTestFinalized event for proper resource management after retries"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR addresses a critical resource management bug where objects were being disposed after each failed test attempt rather than after all retry attempts completed. The solution introduces an OnTestFinalized event that fires in the finally block after all retries are exhausted.

✅ Code Quality & Best Practices

Excellent implementation following TUnit patterns:

• Clean separation of concerns: New event is internal framework-only (TestContextEvents.cs:20)
• Consistent naming: OnTestFinalized follows existing event naming patterns
• Proper documentation: Clear XML comments explaining the event's purpose
• Modern C# usage: Follows repository standards with proper async/await patterns

🐛 Potential Issues

Found one minor issue:

TestCoordinator.cs:106-119: The finalization logic should handle null InvocationList more defensively:

// Current implementation could throw if OnTestFinalized is not null but has no invocations
foreach (var invocation in test.Context.Events.OnTestFinalized.InvocationList.OrderBy(x => x.Order))

Suggestion: Add null check for InvocationList:

if (test.Context.Events.OnTestFinalized?.InvocationList != null)
{
    foreach (var invocation in test.Context.Events.OnTestFinalized.InvocationList.OrderBy(x => x.Order))
    // ...
}

⚡ Performance Considerations

Well optimized:

• ✅ Event fires only once per test (in finally block)
• ✅ Minimal overhead - single event subscription per tracked object
• ✅ No additional allocations in hot paths
• ✅ Proper exception handling prevents performance degradation

🔒 Security Concerns

No security issues identified:

• ✅ Internal event scope prevents external misuse
• ✅ No exposure of sensitive information
• ✅ Exception handling prevents information leakage
• ✅ Follows principle of least privilege

🧪 Test Coverage

Comprehensive test coverage:

ClassDataSourceRetryTests.cs (TUnit.TestProject/Bugs/3219/ClassDataSourceRetryTests.cs:40-87):

• ✅ Tests retry behavior with shared resources
• ✅ Verifies disposal happens only after all attempts
• ✅ Uses dependency testing to verify post-finalization state
• ✅ Includes proper counter tracking and reset logic

Issue3219ReproTest.cs (TUnit.TestProject/Bugs/3219/Issue3219ReproTest.cs:27-44):

• ✅ Simple reproduction case
• ✅ Tests basic retry scenarios
• ✅ Verifies object state consistency across retries

🎯 Architecture Assessment

Excellent architectural decision:

1. Timing: finally block placement ensures finalization happens regardless of test outcome
2. Event placement: TestCoordinator.cs:104-120 is the perfect location for orchestrating finalization
3. ObjectTracker update: ObjectTracker.cs:53 - Clean migration from OnDispose to OnTestFinalized
4. Behavioral parity: Change maintains consistency between source-generated and reflection modes

📝 Minor Suggestions

1. Consider adding unit tests for the OnTestFinalized event itself in isolation
2. The .idea file (copilot.data.migration.ask.xml) should probably be in .gitignore

🏆 Overall Assessment

Excellent fix! This PR:

• ✅ Correctly identifies and fixes the root cause
• ✅ Maintains backward compatibility
• ✅ Follows TUnit's architecture patterns
• ✅ Includes comprehensive test coverage
• ✅ Has minimal performance impact
• ✅ Handles edge cases properly

The implementation is clean, well-documented, and solves the resource management issue elegantly. The only concern is the minor defensive programming opportunity in the TestCoordinator.

Recommendation: Approve with minor suggestion addressed

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3227: "fix: add OnTestFinalized event for proper resource management after retries"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR addresses a critical resource management bug where objects were being disposed after each failed test attempt rather than after all retry attempts completed. The solution introduces an OnTestFinalized event that fires in the finally block after all retries are exhausted.

✅ Code Quality & Best Practices

Excellent implementation following TUnit patterns:

• Clean separation of concerns: New event is internal framework-only (TestContextEvents.cs:20)
• Consistent naming: OnTestFinalized follows existing event naming patterns
• Proper documentation: Clear XML comments explaining the event's purpose
• Modern C# usage: Follows repository standards with proper async/await patterns
• Defensive programming: Null checks and exception handling in place

🐛 Potential Issues

No critical issues found. The implementation is robust:

• ✅ Null safety: Proper null checking before event invocation (TestCoordinator.cs:106)
• ✅ Exception isolation: Try-catch around event invocation prevents failures from affecting test results
• ✅ Deterministic ordering: Uses OrderBy(x => x.Order) for consistent execution order
• ✅ Resource safety: Events fire in finally block ensuring they always execute

⚡ Performance Considerations

Well optimized with minimal overhead:

• ✅ Single execution: Event fires only once per test (in finally block) rather than per retry attempt
• ✅ Efficient tracking: Minimal overhead in ObjectTracker.cs:53 - simple event subscription
• ✅ No hot path allocations: The OrderBy operation has negligible cost for typical event counts
• ✅ Proper async handling: Uses ConfigureAwait(false) appropriately

🔒 Security Concerns

No security issues identified:

• ✅ Access control: internal modifier on OnTestFinalized prevents external misuse
• ✅ Information hiding: Exception handling prevents sensitive information leakage
• ✅ Privilege separation: Event scope is appropriately restricted to framework internals

🧪 Test Coverage

Comprehensive test coverage addressing the root issue:

Primary reproduction test (Issue3219ReproTest.cs:27-44):

• ✅ Tests basic retry scenarios with shared resources
• ✅ Verifies object state consistency across retry attempts
• ✅ Includes console output for debugging verification

Comprehensive verification test (ClassDataSourceRetryTests.cs:40-87):

• ✅ Tests with real IAsyncInitializer and IAsyncDisposable classes
• ✅ Verifies disposal happens only after all attempts complete
• ✅ Uses DependsOn to verify timing and post-finalization state
• ✅ Includes proper counter tracking and session-level verification
• ✅ Tests both during-test and after-session disposal behavior

🎯 Architecture Assessment

Excellent architectural decisions:

1. Event placement: TestCoordinator.cs:104-120 is the perfect orchestration point for finalization
2. Timing: finally block ensures finalization regardless of test outcome
3. ObjectTracker integration: Clean migration from previous event to OnTestFinalized (ObjectTracker.cs:53)
4. Behavioral parity: Change maintains consistency between source-generated and reflection modes
5. Framework encapsulation: Internal event prevents external misuse while solving the core issue

📝 Minor Observations

IDE configuration file: The .idea/copilot.data.migration.ask.xml file is harmless but could be gitignored if not intentionally tracked.

🏆 Overall Assessment

Outstanding fix! This PR:

• ✅ Correctly identifies root cause: Resource disposal timing issue with retries
• ✅ Elegant solution: Simple, focused fix using framework event system
• ✅ Maintains compatibility: No breaking changes, purely additive
• ✅ Follows TUnit patterns: Consistent with existing architecture and coding standards
• ✅ Comprehensive testing: Both reproduction and verification test cases
• ✅ Performance conscious: Minimal overhead aligns with TUnit's performance goals
• ✅ Well documented: Clear comments and XML documentation

The implementation correctly ensures that resource disposal only happens after all retry attempts are complete, which was the core issue reported in #3219. The solution is clean, well-tested, and maintains TUnit's architectural principles.

Recommendation: ✅ APPROVE - This is a high-quality fix ready for merge.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3227: "fix: add OnTestFinalized event for proper resource management after retries"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR addresses a critical resource management bug where objects were being disposed after each failed test attempt rather than after all retry attempts completed. The solution introduces an OnTestFinalized event that fires in the finally block after all retries are exhausted.

✅ Code Quality & Best Practices

Excellent implementation following TUnit patterns:

• Clean separation of concerns: New event is internal framework-only (TestContextEvents.cs:20)
• Consistent naming: OnTestFinalized follows existing event naming patterns
• Proper documentation: Clear XML comments explaining the event's purpose
• Modern C# usage: Follows repository standards with proper async/await patterns
• Defensive programming: Null checks and exception handling in place

🐛 Potential Issues

No critical issues found. The implementation is robust:

• ✅ Null safety: Proper null checking before event invocation (TestCoordinator.cs:106)
• ✅ Exception isolation: Try-catch around event invocation prevents failures from affecting test results
• ✅ Deterministic ordering: Uses OrderBy(x => x.Order) for consistent execution order
• ✅ Resource safety: Events fire in finally block ensuring they always execute

⚡ Performance Considerations

Well optimized with minimal overhead:

• ✅ Single execution: Event fires only once per test (in finally block) rather than per retry attempt
• ✅ Efficient tracking: Minimal overhead in ObjectTracker.cs:53 - simple event subscription
• ✅ No hot path allocations: The OrderBy operation has negligible cost for typical event counts
• ✅ Proper async handling: Uses ConfigureAwait(false) appropriately

🔒 Security Concerns

No security issues identified:

• ✅ Access control: internal modifier on OnTestFinalized prevents external misuse
• ✅ Information hiding: Exception handling prevents sensitive information leakage
• ✅ Privilege separation: Event scope is appropriately restricted to framework internals

🧪 Test Coverage

Comprehensive test coverage addressing the root issue:

Primary reproduction test (Issue3219ReproTest.cs:27-44):

• ✅ Tests basic retry scenarios with shared resources
• ✅ Verifies object state consistency across retry attempts
• ✅ Includes console output for debugging verification

Comprehensive verification test (ClassDataSourceRetryTests.cs:40-87):

• ✅ Tests with real IAsyncInitializer and IAsyncDisposable classes
• ✅ Verifies disposal happens only after all attempts complete
• ✅ Uses DependsOn to verify timing and post-finalization state
• ✅ Includes proper counter tracking and session-level verification
• ✅ Tests both during-test and after-session disposal behavior

🎯 Architecture Assessment

Excellent architectural decisions:

1. Event placement: TestCoordinator.cs:104-120 is the perfect orchestration point for finalization
2. Timing: finally block ensures finalization regardless of test outcome
3. ObjectTracker integration: Clean migration from previous event to OnTestFinalized (ObjectTracker.cs:53)
4. Behavioral parity: Change maintains consistency between source-generated and reflection modes
5. Framework encapsulation: Internal event prevents external misuse while solving the core issue

📝 Minor Observations

IDE configuration file: The .idea/copilot.data.migration.ask.xml file is harmless but could be gitignored if not intentionally tracked.

🏆 Overall Assessment

Outstanding fix! This PR:

• ✅ Correctly identifies root cause: Resource disposal timing issue with retries
• ✅ Elegant solution: Simple, focused fix using framework event system
• ✅ Maintains compatibility: No breaking changes, purely additive
• ✅ Follows TUnit patterns: Consistent with existing architecture and coding standards
• ✅ Comprehensive testing: Both reproduction and verification test cases
• ✅ Performance conscious: Minimal overhead aligns with TUnit's performance goals
• ✅ Well documented: Clear comments and XML documentation

The implementation correctly ensures that resource disposal only happens after all retry attempts are complete, which was the core issue reported in #3219. The solution is clean, well-tested, and maintains TUnit's architectural principles.

Recommendation: ✅ APPROVE - This is a high-quality fix ready for merge.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3227: "fix: add OnTestFinalized event for proper resource management after retries"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR addresses a critical resource management bug where objects were being disposed after each failed test attempt rather than after all retry attempts completed. The solution introduces an OnTestFinalized event that fires in the finally block after all retries are exhausted.

✅ Code Quality & Best Practices

Excellent implementation following TUnit patterns:

• Clean separation of concerns: New event is internal framework-only (TestContextEvents.cs:20)
• Consistent naming: OnTestFinalized follows existing event naming patterns
• Proper documentation: Clear XML comments explaining the event's purpose
• Modern C# usage: Follows repository standards with proper async/await patterns
• Defensive programming: Null checks and exception handling in place

🐛 Potential Issues

No critical issues found. The implementation is robust:

• ✅ Null safety: Proper null checking before event invocation (TestCoordinator.cs:123)
• ✅ Exception isolation: Try-catch around event invocation prevents failures from affecting test results
• ✅ Deterministic ordering: Uses OrderBy(x => x.Order) for consistent execution order
• ✅ Resource safety: Events fire in finally block ensuring they always execute

⚡ Performance Considerations

Well optimized with minimal overhead:

• ✅ Single execution: Event fires only once per test (in finally block) rather than per retry attempt
• ✅ Efficient tracking: Minimal overhead in ObjectTracker.cs:53 - simple event subscription
• ✅ No hot path allocations: The OrderBy operation has negligible cost for typical event counts
• ✅ Proper async handling: Uses ConfigureAwait(false) appropriately

🔒 Security Concerns

No security issues identified:

• ✅ Access control: internal modifier on OnTestFinalized prevents external misuse
• ✅ Information hiding: Exception handling prevents sensitive information leakage
• ✅ Privilege separation: Event scope is appropriately restricted to framework internals

🧪 Test Coverage

Comprehensive test coverage addressing the root issue:

Primary reproduction test (Issue3219ReproTest.cs:27-44):

• ✅ Tests basic retry scenarios with shared resources
• ✅ Verifies object state consistency across retry attempts
• ✅ Includes console output for debugging verification

Comprehensive verification test (ClassDataSourceRetryTests.cs:40-87):

• ✅ Tests with real IAsyncInitializer and IAsyncDisposable classes
• ✅ Verifies disposal happens only after all attempts complete
• ✅ Uses DependsOn to verify timing and post-finalization state
• ✅ Includes proper counter tracking and session-level verification
• ✅ Tests both during-test and after-session disposal behavior

🎯 Architecture Assessment

Excellent architectural decisions:

1. Event placement: TestCoordinator.cs:122-136 is the perfect orchestration point for finalization
2. Timing: finally block ensures finalization regardless of test outcome
3. ObjectTracker integration: Clean migration from previous event to OnTestFinalized (ObjectTracker.cs:53)
4. Behavioral parity: Change maintains consistency between source-generated and reflection modes
5. Framework encapsulation: Internal event prevents external misuse while solving the core issue

📝 Minor Observations

Debug/output files: Several debug and output files were added (debug_output.txt, test_output.txt, etc.) that appear to be for debugging purposes and could potentially be gitignored.

🏆 Overall Assessment

Outstanding fix! This PR:

• ✅ Correctly identifies root cause: Resource disposal timing issue with retries
• ✅ Elegant solution: Simple, focused fix using framework event system
• ✅ Maintains compatibility: No breaking changes, purely additive
• ✅ Follows TUnit patterns: Consistent with existing architecture and coding standards
• ✅ Comprehensive testing: Both reproduction and verification test cases
• ✅ Performance conscious: Minimal overhead aligns with TUnit's performance goals
• ✅ Well documented: Clear comments and XML documentation

The implementation correctly ensures that resource disposal only happens after all retry attempts are complete, which was the core issue reported in #3219. The solution is clean, well-tested, and maintains TUnit's architectural principles.

Recommendation: ✅ APPROVE - This is a high-quality fix ready for merge.

---