fix: Remove unnecessary finalizer from ModuleOutputWriter

[thomhurst]

Summary

• Removes the finalizer since the class only manages managed resources (IServiceScope)
• Fixes a bug where _scope was not disposed when the buffer had no events
• Removes GC.SuppressFinalize call since no finalizer exists

Why the finalizer was problematic

1. Unsafe: The finalizer called Dispose() which accessed managed objects that may already be finalized
2. Locking in finalizer: Taking locks in finalizers is risky and can cause hangs
3. Unnecessary: The class only manages managed resources, so a finalizer adds no value

Fixes #1533

Test plan

• Existing unit tests pass
• Module output displays correctly

🤖 Generated with Claude Code

[thomhurst]

Summary

Removes unsafe finalizer from ModuleOutputWriter and fixes resource leak where IServiceScope wasn't disposed in early return path.

Critical Issues

None found ✅

Suggestions

None - the changes are correct and well-documented.

Analysis

The PR correctly addresses the finalizer issues:

1. Finalizer removal is appropriate: The class only manages IServiceScope (a managed resource), so a finalizer adds no value and is actually harmful:

   • Accessing managed objects from finalizers is unsafe (they may already be finalized)
   • Taking locks in finalizers can cause deadlocks

2. Resource leak fix: The early return path at line 136 now properly disposes the scope, preventing a leak when the buffer has no events.

3. Dispose pattern: The two _scope.Dispose() calls (lines 136 and 180) are in mutually exclusive code paths, so the scope is correctly disposed exactly once in all scenarios.

4. Documentation: The added XML remarks clearly explain the design decision.

Verdict

✅ APPROVE - No critical issues

[Copilot]

Pull request overview

This PR removes the problematic finalizer from ModuleOutputWriter and fixes a resource leak where the IServiceScope was not disposed when the buffer had no events.

Key changes:

• Removed the finalizer that unsafely called Dispose()
• Fixed bug where _scope.Dispose() was not called in the early return path when buffer has no events
• Removed unnecessary GC.SuppressFinalize(this) call since no finalizer exists