Add dispose exception observability via UnobservedDisposeException

[imurashka]

What changed

Added ControllerBase.UnobservedDisposeException — a static event on ControllerBase. Projects can subscribe to it on app start and route dispose exceptions to their own logger, crashlytics, etc.

How exception aggregation works

Added ExecuteAndCollectExceptions utility method (internal static on ControllerBase) that:

• executes an action in try-catch
• recursively flattens any AggregateException down to leaf exceptions
• raises UnobservedDisposeException for each leaf that wasn't already raised
• ControllerDisposeAggregateException is an internal marker — its children were already raised, so they won't be raised again in outer composites

This guarantees each leaf exception is raised via event exactly once, even in deeply nested composite hierarchies.

Both ControllerBase.DisposeInternal() and ControllerCompositeDisposable.DisposeMany() use this shared method.

Other changes

• Fixed pool leak in ControllerBase.DisposeInternal — if _disposables.Dispose() threw, ListPool<IController>.Release and _state = Disposed were skipped. Now wrapped in try/finally.
• Early return optimization for DisposeMany when collection is empty.
• Removed lazy ControllerCompositeDisposable creation — Initialize() always calls AddDisposable(_lifetimeTokenSource), so it's created every time anyway. Simpler to just create it eagerly.

Tests

• Added ControllerBaseExceptionsTests — 8 tests for ExecuteAndCollectExceptions covering plain exceptions, AggregateException, ControllerDisposeAggregateException, nested mixed hierarchies, no handler scenario
• Updated existing tests for ControllerDisposeAggregateException
• Added nested composite tests that verify each leaf is raised exactly once

README

Added "Handling Dispose Exceptions" section to README.md with setup example.

Summary by CodeRabbit

Release Notes

• New Features

  • Added public event to notify about unobserved exceptions during disposal.
  • Enhanced exception aggregation and collection during resource cleanup.
  • Added public methods to manage disposable resources.

• Documentation

  • Added new section covering handling of disposal exceptions.

• Tests

  • Added comprehensive test coverage for exception handling during disposal and resource cleanup operations.

[eldarmguseynov]

Thank you for the fix. What do you think about optimizations around disposables?

[imurashka]

Thank you for the fix. What do you think about optimizations around disposables?

I will be honest with you, I didn't think about optimization here too much. Right now, it looks pretty optimized, as I see. Espesially after small tweaks you proposed in method DisposeMany and ControllerBase class.

[imurashka]

@eldarmguseynov hello! Is there any chance that you will merge this anytime soon?

[perepechenko]

Related to https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1065 Dispose should not throw any exception.

As for the use of Debug.LogError, many projects use their own logger, and we avoided using Debug.LogError in the controllers core.

The idea of a lazy composite disposable object is good, and we can use pooling instead of new.

@eldarmguseynov what do you think?

[imurashka]

hey, @perepechenko, thanks for the review!

so here is what i changed based on it:

Debug.LogException removed from core. replaced with a static event ControllerBase.UnobservedDisposeException. projects subscribe to it on app start and route exceptions wherever they want — Debug.LogException, crashlytics, custom logger, whatever.

went with static event on ControllerBase because it's simple and follows existing patterns in .NET and Unity — like TaskScheduler.UnobservedTaskException, UniTaskScheduler.UnobservedTaskException, Application.logMessageReceived. the event lives on the main type users interact with, so it's easy to discover. no need to pass handler instances through constructors or factories for exactly this case.

about lazy composite disposable — i added it, then removed it. Initialize() always calls AddDisposable(_lifetimeTokenSource) which triggers composite creation anyway. so lazy init saves zero allocations in practice. rolled it back to keep code simpler.

about pooling composite disposable — i think it makes sense but want to do it in a separate PR to keep this one focused on the exception handling change.

also fixed a bug in DisposeInternal — if dispose threw, ListPool.Release and _state = Disposed were skipped (no finally block). now fixed.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds disposal exception aggregation and handling across controller disposal paths: new ControllerDisposeAggregateException, utilities to execute and flatten exceptions (ExecuteAndCollectExceptions, CollectExceptions), event UnobservedDisposeException, integration into DisposeInternal and ControllerCompositeDisposable, expanded unit tests, metadata, and an added README section (duplicated).

Changes

Cohort / File(s)	Summary
Exception handling core src/ControllersTree/Core/Controllers/ControllerBase.Exceptions.cs	Introduces UnobservedDisposeException event, RaiseUnhandledDisposeException, ExecuteAndCollectExceptions, and CollectExceptions to flatten AggregateExceptions and collect leaf exceptions while raising events for unobserved leaves.
Controller disposal integration src/ControllersTree/Core/Controllers/ControllerBase.cs	DisposeInternal now uses a pooled exception list and ExecuteAndCollectExceptions for lifetime token cancellation and composite disposal; throws ControllerDisposeAggregateException if any leaves collected; finally block always releases children and sets state.
Composite disposable changes src/ControllersTree/Core/Utils/ControllerCompositeDisposable.cs	Adds ControllerDisposeAggregateException type; new public Add and AddRange methods; refactors disposal to DisposeMany(IReadOnlyCollection<IDisposable>) and uses ExecuteAndCollectExceptions to collect exceptions instead of immediate propagation.
Tests src/ControllersTree/Tests/ControllerBaseExceptionsTests.cs, src/ControllersTree/Tests/ControllerCompositeDisposableTests.cs, src/ControllersTree/Tests/ControllersWithResultBaseTests.cs	Adds comprehensive tests for exception collection/flattening and event raising; updates composite-disposable tests to assert new aggregate exception behavior; adjusts one test to expect AggregateException.
Metadata & config src/ControllersTree/Core/AssemblyInfo.cs, src/ControllersTree/Core/Controllers/ControllerBase.Exceptions.cs.meta, src/ControllersTree/Tests/ControllerBaseExceptionsTests.cs.meta, README.md	Adds InternalsVisibleTo for test assembly; adds .meta files for new tests; inserts “Handling Dispose Exceptions” section into README (appears duplicated).

Sequence Diagram

sequenceDiagram
    participant Caller as Client
    participant CB as ControllerBase
    participant Pool as ListPool<Exception>
    participant ECAE as ExecuteAndCollectExceptions
    participant COL as CollectExceptions
    participant Handler as UnobservedDisposeExceptionHandler

    Caller->>CB: Dispose()
    CB->>Pool: Rent exception list
    CB->>ECAE: ExecuteAndCollectExceptions(list, cancel lifetime token)
    ECAE->>ECAE: run action
    ECAE-->>COL: exception
    COL->>COL: flatten AggregateException -> collect leaves
    COL->>Handler: raise UnobservedDisposeException for each new leaf
    Handler-->>COL: handler runs
    COL-->>ECAE: return with leaves collected
    CB->>ECAE: ExecuteAndCollectExceptions(list, dispose composite)
    ECAE->>ECAE: run action
    ECAE-->>COL: exception
    COL->>COL: flatten & collect leaves, raise events as needed
    COL-->>ECAE: return
    alt list not empty
        CB->>CB: throw ControllerDisposeAggregateException(list)
    end
    CB->>CB: finally: release children, set state Disposed
    CB->>Pool: Return exception list

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 I hopped through disposals, fluff and thread,
Collected leaves of errors that once fled.
I raised a bell for each unobserved plight,
Bundled them snug, then set things right.
Tests in my burrow prove the fix is bright.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.13% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the primary change: adding an UnobservedDisposeException event to enable customizable exception handling during disposal.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/ControllersTree/Tests/ControllersWithResultBaseTests.cs (1)

218-228: ⚠️ Potential issue | 🟡 Minor

Assertion message is inconsistent with the expected exception type.

The test now catches AggregateException but the assertion message on line 227 still says "TestControllersException expected". Consider updating the message for clarity:

📝 Suggested fix

-            Assert.IsTrue(exceptionThrown, "TestControllersException expected");
+            Assert.IsTrue(exceptionThrown, "AggregateException expected");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ControllersTree/Tests/ControllersWithResultBaseTests.cs` around lines 218
- 228, The assertion message is misleading: the catch now handles
AggregateException but Assert.IsTrue(exceptionThrown, "TestControllersException
expected") still references TestControllersException; update the assertion
message in ControllersWithResultBaseTests (the test method containing the catch
for AggregateException) to reflect AggregateException (e.g., "AggregateException
expected") or change the test to assert the specific exception type via
Assert.Throws if you prefer a stronger check; locate the
Assert.IsTrue(exceptionThrown, ...) and replace the message accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ControllersTree/Core/Controllers/ControllerBase.Exceptions.cs`:
- Around line 16-19: RaiseUnhandledDisposeException currently invokes
UnobservedDisposeException directly which lets a subscriber exception escape and
abort CollectExceptions/DisposeMany; change it to retrieve
UnobservedDisposeException?.GetInvocationList() and invoke each delegate in a
try/catch so subscriber exceptions are caught and do not propagate (optionally
log or aggregate them) ensuring no exception escapes
RaiseUnhandledDisposeException and the exception-collection loop in
CollectExceptions can continue. Include references to
UnobservedDisposeException, RaiseUnhandledDisposeException, CollectExceptions,
and DisposeMany when making the change.

---

Outside diff comments:
In `@src/ControllersTree/Tests/ControllersWithResultBaseTests.cs`:
- Around line 218-228: The assertion message is misleading: the catch now
handles AggregateException but Assert.IsTrue(exceptionThrown,
"TestControllersException expected") still references TestControllersException;
update the assertion message in ControllersWithResultBaseTests (the test method
containing the catch for AggregateException) to reflect AggregateException
(e.g., "AggregateException expected") or change the test to assert the specific
exception type via Assert.Throws if you prefer a stronger check; locate the
Assert.IsTrue(exceptionThrown, ...) and replace the message accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f9adf36d-c9e1-4686-8b14-0beced5df591

📥 Commits

Reviewing files that changed from the base of the PR and between 5c07876 and 43701e6.

📒 Files selected for processing (10)

• README.md
• src/ControllersTree/Core/AssemblyInfo.cs
• src/ControllersTree/Core/Controllers/ControllerBase.Exceptions.cs
• src/ControllersTree/Core/Controllers/ControllerBase.Exceptions.cs.meta
• src/ControllersTree/Core/Controllers/ControllerBase.cs
• src/ControllersTree/Core/Utils/ControllerCompositeDisposable.cs
• src/ControllersTree/Tests/ControllerBaseExceptionsTests.cs
• src/ControllersTree/Tests/ControllerBaseExceptionsTests.cs.meta
• src/ControllersTree/Tests/ControllerCompositeDisposableTests.cs
• src/ControllersTree/Tests/ControllersWithResultBaseTests.cs