Bug: EntityBroker subscriptions leak — SubscribedGet/SubscribedQuery have no disposal API, abandoned subscriptions accumulate and fire real network requests

Symptom

In a production session, the entity broker accumulates 211+ active get requests firing on every entity update. Each request is a real DAL/network round-trip, causing quadratic load as the session continues.

Root cause

SubscribedGet and SubscribedQuery implement no IDisposable interface and have no explicit deregistration API. The broker stores them as WeakReference<T> in internal lists:

// EntityBroker.cs lines 17–19
private readonly List<WeakReference<SubscribedGet>>   subscribedGets    = new();
private readonly List<WeakReference<SubscribedQuery>> subscribedQueries = new();

The only cleanup path is GC-based compaction during RefreshSubscribedGetsAsync: entries whose WeakReference.TryGetTarget returns false are dropped. Until the GC collects the object, every abandoned subscription fires RefreshAsync() → a real DataAccessLayer.GetAsync() call on every broker update cycle.

In a long-running production session with low memory pressure, the GC rarely runs eagerly enough to collect abandoned subscriptions, so they accumulate until memory runs low or the session is restarted.

Confirmed leak sites

1. View population — `MainWindowViewModel.ApplySelectedViewAsync` (lines 743–810)

// Lines 746–747 — .Clear() does NOT dispose
this.selectedViewSubViewSubscriptions.Clear();
this.selectedViewSubViewQuerySubscriptions.Clear();

// Lines 841–905 — new subscriptions created every view switch
var subscribedGet = await this.EntityBroker.SubscribeGetAsync(getRequest);
this.selectedViewSubViewSubscriptions.Add(subscribedGet);

var subscribedQuery = await this.EntityBroker.SubscribeQueryAsync(effectiveQuery);
this.selectedViewSubViewQuerySubscriptions.Add(subscribedQuery);

var noteSubscription = await this.EntityBroker.SubscribeGetAsync(…);
this.selectedViewSubViewSubscriptions.Add(noteSubscription);

Each view switch creates 2–3 new subscriptions and drops the previous ones into GC limbo. After ~70 view navigations the broker holds 140–210+ abandoned subscriptions. MainWindowViewModel.DisposeAsync never touches these lists.

Trigger sites for ApplySelectedViewAsync: ShowHiddenItems setter, SelectedTopLevelView setter, OnEntityBrokerChanged, OnInterestCatalogChanged, OnEntityTypeCatalogChanged — each fires the method concurrently with no cancellation (see also #104).

2. Entity browser tab close — `EntityBrowserWorkspaceTabViewModel`

// Class declaration — no IDisposable
public sealed class EntityBrowserWorkspaceTabViewModel : WorkspaceTabViewModel

Fields that hold live subscriptions:

rootSubscribedGet: SubscribedGet (line 20) — created on construction, never disposed
subscribedGetsByPath: Dictionary<string, SubscribedGet> (line 22) — one entry per expanded tree node

WorkspaceTabViewModel base class has no disposal interface. DisposeWorkspaceTab in MainWindowViewModel (lines 1748–1759) checks for IAsyncDisposable/IDisposable and falls through silently for EntityBrowserWorkspaceTabViewModel. When the entity browser tab is closed, all its subscriptions are abandoned.

3. Duplicate entity browser open — `OpenEntityBrowserTabAsync` (lines 1762–1793)

var subscribedGet = await this.EntityBroker.SubscribeGetAsync(…);  // always created
var entityBrowserTab = new EntityBrowserWorkspaceTabViewModel(this.EntityBroker, subscribedGet);
await this.OpenTabAsync(entityBrowserTab);  // if tab already open, disposes new tab

OpenTabAsync detects a duplicate and calls DisposeWorkspaceTab(entityBrowserTab) on the just-created instance — which does nothing (no IDisposable). The subscribedGet lives until both the transient entityBrowserTab and the subscribedGet it holds are GC'd.

Fix: check for an existing entity browser tab before calling SubscribeGetAsync.

4. `ViewHierarchyAssembler.cs` line 182

var subscription = await this.entityBroker.SubscribeQueryAsync(query, cancellationToken);
return subscription.Results.ToList();

Results are read immediately and subscription goes out of scope. It is a transient per-call leak — each call to ViewHierarchyAssembler leaves one SubscribedQuery in the broker until GC runs. If GetEntitiesAsync (non-subscribing) meets the need here, replace it.

Fix direction

1. Add `IDisposable` to `SubscribedGet` and `SubscribedQuery`

Expose an internal Unregister callback on EntityBroker:

public sealed class SubscribedGet : IDisposable
{
    private readonly Action _unregister;
    internal SubscribedGet(Action unregister, …) { _unregister = unregister; }
    public void Dispose() => _unregister();
}

EntityBroker.SubscribeGetAsync passes a lambda that removes the WeakReference from the list (under the existing gate lock). Same pattern for SubscribedQuery.

2. Dispose subscriptions before clearing lists in `ApplySelectedViewAsync`

foreach (var s in this.selectedViewSubViewSubscriptions)      s.Dispose();
foreach (var s in this.selectedViewSubViewQuerySubscriptions) s.Dispose();
this.selectedViewSubViewSubscriptions.Clear();
this.selectedViewSubViewQuerySubscriptions.Clear();

This will be superseded by the ViewPopulationViewModel refactor (#104), whose DisposeAsync will do this cleanup automatically.

3. Implement `IAsyncDisposable` on `EntityBrowserWorkspaceTabViewModel`

public ValueTask DisposeAsync()
{
    this.rootSubscribedGet.Dispose();
    foreach (var s in this.subscribedGetsByPath.Values) s.Dispose();
    this.subscribedGetsByPath.Clear();
    return ValueTask.CompletedTask;
}

4. Fix early-exit in `OpenEntityBrowserTabAsync`

Check for an existing tab before calling SubscribeGetAsync:

if (this.FindExistingTab("entity-browser-tab") is { } existing)
{
    this.ActivateTab(existing);
    return;
}
var subscribedGet = await this.EntityBroker.SubscribeGetAsync(…);
…

5. Replace `SubscribeQueryAsync` with `GetEntitiesAsync` in `ViewHierarchyAssembler`

If live updates are not needed at that call site, use the non-subscribing path to avoid adding broker entries at all.

Files

File	Issue
`Phantom.Workspaces.Data.Core/EntityBroker.cs`	`SubscribedGet`/`SubscribedQuery` lack `IDisposable`; no explicit deregistration
`Phantom.Workspaces/ViewModels/MainWindowViewModel.cs`	Lines 746–747 clear without dispose; `DisposeAsync` misses subscription lists
`Phantom.Workspaces/ViewModels/EntityBrowserWorkspaceTabViewModel.cs`	No `IDisposable`; subscriptions leak on tab close
`Phantom.Workspaces/ViewModels/ViewHierarchyAssembler.cs`	Line 182 creates transient `SubscribedQuery` and immediately drops reference

Missing GC-cleanup tests for `SubscribedGet` and `SubscribedQuery`

EntityBrokerTests.cs already has a RefreshAsync_SkipsCollectedSubscriptions test (line 96) that proves collected SubscribedEntityViewModel entries are dropped from the broker. The required pattern ([NoInlining] helper, ForceGarbageCollection, WeakReference liveness check) is already in place:

[MethodImpl(MethodImplOptions.NoInlining)]
private static async Task<WeakReference<SubscribedEntityViewModel>> CreateCollectedEntityAsync(…)

private static void ForceGarbageCollection()
{
    GC.Collect();
    GC.WaitForPendingFinalizers();
    GC.Collect();
}

There are no equivalent tests for SubscribedGet or SubscribedQuery. The following tests must be added:

`RefreshAsync_SkipsCollectedSubscribedGet`

[AvaloniaFact]
public async Task RefreshAsync_SkipsCollectedSubscribedGet()
{
    var broker = await CreateBrokerAsync(ct);
    // seed some entities …

    var weakRef = await CreateCollectedSubscribedGetAsync(broker, request);

    ForceGarbageCollection();
    Assert.False(weakRef.TryGetTarget(out _));   // must be dead

    // RefreshAsync must not throw and must not attempt to refresh the dead subscription
    await broker.RefreshAsync(ct);

    // After compaction the broker's internal list should contain 0 SubscribedGet entries
    Assert.Equal(0, broker.ActiveSubscribedGetCount);   // see note below
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static async Task<WeakReference<SubscribedGet>> CreateCollectedSubscribedGetAsync(
    EntityBroker broker, GetRequest request)
{
    var sub = await broker.SubscribeGetAsync(request, TestContext.Current.CancellationToken);
    return new WeakReference<SubscribedGet>(sub);
    // sub goes out of scope here; NoInlining prevents JIT from keeping it on the caller stack
}

`RefreshAsync_SkipsCollectedSubscribedQuery`

Same shape, using SubscribeQueryAsync and ActiveSubscribedQueryCount.

Expose internal count properties for testability

To make these tests precise (not just "does not throw"), EntityBroker needs observable count properties. Add to EntityBroker.cs:

// For tests only — counts live (non-collected) WeakReference entries after compaction
internal int ActiveSubscribedGetCount
{
    get
    {
        lock (this.gate)
            return this.subscribedGets.Count(r => r.TryGetTarget(out _));
    }
}

internal int ActiveSubscribedQueryCount
{
    get
    {
        lock (this.gate)
            return this.subscribedQueries.Count(r => r.TryGetTarget(out _));
    }
}

These let tests assert compaction actually happened rather than only verifying "no exception".

Why `[NoInlining]` is essential

Without [NoInlining] on the helper that creates the subscription, the JIT may keep the local sub variable alive in a register across the GC.Collect() call in the caller, causing WeakReference.TryGetTarget to still return true and the test to give a false negative. The existing CreateCollectedEntityAsync already uses this pattern correctly.

Note on the `IDisposable` fix interaction

Once IDisposable is added to SubscribedGet/SubscribedQuery (as described in the main bug), the preferred test becomes:

[AvaloniaFact]
public async Task Dispose_RemovesSubscribedGetFromBroker()
{
    var broker = await CreateBrokerAsync(ct);
    var sub = await broker.SubscribeGetAsync(request, ct);
    Assert.Equal(1, broker.ActiveSubscribedGetCount);

    sub.Dispose();

    Assert.Equal(0, broker.ActiveSubscribedGetCount);
}

Both the GC-path test and the explicit-dispose test should be present: the GC test documents the fallback safety net, the dispose test documents the expected fast path.

I don't think Dispose is the way to go. I think just cleaning up collected subscriptions ought to be fine. If necessary, current users can null-out their references to subscriptions. I'd like duplicate subscriptions to be able to be reused, so disposing of them negates that.

Understood — removing the IDisposable direction. Updated approach:

No IDisposable on SubscribedGet/SubscribedQuery. GC-based cleanup via WeakReference remains the only removal mechanism. The fix at each leak site is to null out (or stop holding) the strong reference as soon as the subscription is no longer needed, so GC can collect it promptly:

ApplySelectedViewAsync: instead of .Clear() on the subscription lists (which drops list references but the list itself is a GC root keeping objects alive), null out / replace individual field references or let ViewPopulationViewModel (Bug: view entities double-populate when navigating away and back (concurrent ApplySelectedViewAsync) #104) go out of scope entirely when replaced.
EntityBrowserWorkspaceTabViewModel tab close: set rootSubscribedGet = null and clear subscribedGetsByPath so no strong references remain.
OpenEntityBrowserTabAsync: early-exit before calling SubscribeGetAsync if a live tab already exists.

Deduplication: if the same GetRequest is re-requested and the existing SubscribedGet is still alive (via WeakReference.TryGetTarget), return the existing instance rather than creating a new one. This prevents accumulation when callers re-subscribe to the same view repeatedly. Requires a stable key for GetRequest (equality or a canonical string) to look up the existing weak ref.

Test plan stays the same but drops the explicit-dispose test. Only the GC-path tests remain:

RefreshAsync_SkipsCollectedSubscribedGet
RefreshAsync_SkipsCollectedSubscribedQuery

Both using the [NoInlining] + ForceGarbageCollection + WeakReference liveness pattern, asserting ActiveSubscribedGetCount == 0 after collection and RefreshAsync.

I don't think we need null-out subscription lists. Clearing out the list removes the references.

EntityBrowserWorkspaceTabViewModel tab close - doesn't this eventually free up the EntityBrowse4rWorkspaceTabViewModel, too?

The key for GetRequest, etc., can just be the serialized json content of the request.

Fixed in commit 17e147d. Changes:

EntityBroker: Replaced List<WeakReference> with Dictionary<string, WeakReference> (key = JSON-serialized request) for deduplication — same request reuses the alive subscription
EntityBroker: Added GetEntitiesAsync(QueryRequest) non-subscribing overload
EntityBroker: Added internal ActiveSubscribedGetCount/ActiveSubscribedQueryCount for test observability
ViewHierarchyAssembler: Replaced transient SubscribeQueryAsync (line 182) with GetEntitiesAsync — no broker accumulation for per-call hierarchy traversal
MainWindowViewModel.OpenEntityBrowserTabAsync: Early-exit before SubscribeGetAsync when entity browser tab already open
Tests: Added RefreshAsync_SkipsCollectedSubscribedGet and RefreshAsync_SkipsCollectedSubscribedQuery using [NoInlining] + ForceGarbageCollection + WeakReference liveness pattern

Fix #106: deduplicate SubscribedGet/Query with WeakReference dictiona…

Verification passed

Checked: Commit 17e147d on the \eatures\ branch, files:

\Phantom.Workspaces/EntityBroker.cs\
\Phantom.Workspaces/ViewHierarchyAssembler.cs\
\Phantom.Workspaces/ViewModels/MainWindowViewModel.cs\
\Phantom.Workspaces.Tests/EntityBrokerTests.cs\

Result: Implementation found, all verification criteria satisfied, tests present.

What was verified

Fix	Location	Status
Replace \List<WeakReference>\ with \Dictionary<string, WeakReference>\ keyed by JSON-serialized request	\EntityBroker.cs\	✅
Deduplication in \SubscribeGetAsync\ / \SubscribeQueryAsync\ — returns existing alive subscription for same request	\EntityBroker.cs\	✅
Dead-key removal during compaction (replaces list Clear/AddRange)	\EntityBroker.cs\	✅
\GetEntitiesAsync(QueryRequest)\ non-subscribing overload	\EntityBroker.cs\	✅
\ActiveSubscribedGetCount\ / \ActiveSubscribedQueryCount\ internal properties for test observability	\EntityBroker.cs\	✅
Replace transient \SubscribeQueryAsync\ with \GetEntitiesAsync\ (line 182 leak)	\ViewHierarchyAssembler.cs\	✅
Early-exit before \SubscribeGetAsync\ when entity browser tab already open	\MainWindowViewModel.OpenEntityBrowserTabAsync\	✅
\RefreshAsync_SkipsCollectedSubscribedGet\ GC-path test with [NoInlining]\ + \ForceGarbageCollection\ + \WeakReference\ liveness	\EntityBrokerTests.cs\	✅
\RefreshAsync_SkipsCollectedSubscribedQuery\ GC-path test (same pattern)	\EntityBrokerTests.cs\	✅

No disabled/skipped tests, no unresolved TODOs, no timing-dependent synchronisation found.

Navigation Menu

Repository navigation

Bug: EntityBroker subscriptions leak — SubscribedGet/SubscribedQuery have no disposal API, abandoned subscriptions accumulate and fire real network requests #106

Symptom

Root cause

Confirmed leak sites

1. View population — `MainWindowViewModel.ApplySelectedViewAsync` (lines 743–810)

2. Entity browser tab close — `EntityBrowserWorkspaceTabViewModel`

3. Duplicate entity browser open — `OpenEntityBrowserTabAsync` (lines 1762–1793)

4. `ViewHierarchyAssembler.cs` line 182

Fix direction

1. Add `IDisposable` to `SubscribedGet` and `SubscribedQuery`

2. Dispose subscriptions before clearing lists in `ApplySelectedViewAsync`

3. Implement `IAsyncDisposable` on `EntityBrowserWorkspaceTabViewModel`

4. Fix early-exit in `OpenEntityBrowserTabAsync`

5. Replace `SubscribeQueryAsync` with `GetEntitiesAsync` in `ViewHierarchyAssembler`

Files

Missing GC-cleanup tests for `SubscribedGet` and `SubscribedQuery`

`RefreshAsync_SkipsCollectedSubscribedGet`

`RefreshAsync_SkipsCollectedSubscribedQuery`

Expose internal count properties for testability

Why `[NoInlining]` is essential

Note on the `IDisposable` fix interaction

Verification passed

What was verified

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Bug: EntityBroker subscriptions leak — SubscribedGet/SubscribedQuery have no disposal API, abandoned subscriptions accumulate and fire real network requests #106

Description

Symptom

Root cause

Confirmed leak sites

1. View population — MainWindowViewModel.ApplySelectedViewAsync (lines 743–810)

2. Entity browser tab close — EntityBrowserWorkspaceTabViewModel

3. Duplicate entity browser open — OpenEntityBrowserTabAsync (lines 1762–1793)

4. ViewHierarchyAssembler.cs line 182

Fix direction

1. Add IDisposable to SubscribedGet and SubscribedQuery

2. Dispose subscriptions before clearing lists in ApplySelectedViewAsync

3. Implement IAsyncDisposable on EntityBrowserWorkspaceTabViewModel

4. Fix early-exit in OpenEntityBrowserTabAsync

5. Replace SubscribeQueryAsync with GetEntitiesAsync in ViewHierarchyAssembler

Files

Activity

JoshuaRowePhantom commented on Jun 26, 2026

Missing GC-cleanup tests for SubscribedGet and SubscribedQuery

RefreshAsync_SkipsCollectedSubscribedGet

RefreshAsync_SkipsCollectedSubscribedQuery

Expose internal count properties for testability

Why [NoInlining] is essential

Note on the IDisposable fix interaction

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 26, 2026

JoshuaRowePhantom commented on Jun 30, 2026

Verification passed

What was verified

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

1. View population — `MainWindowViewModel.ApplySelectedViewAsync` (lines 743–810)

2. Entity browser tab close — `EntityBrowserWorkspaceTabViewModel`

3. Duplicate entity browser open — `OpenEntityBrowserTabAsync` (lines 1762–1793)

4. `ViewHierarchyAssembler.cs` line 182

1. Add `IDisposable` to `SubscribedGet` and `SubscribedQuery`

2. Dispose subscriptions before clearing lists in `ApplySelectedViewAsync`

3. Implement `IAsyncDisposable` on `EntityBrowserWorkspaceTabViewModel`

4. Fix early-exit in `OpenEntityBrowserTabAsync`

5. Replace `SubscribeQueryAsync` with `GetEntitiesAsync` in `ViewHierarchyAssembler`

Missing GC-cleanup tests for `SubscribedGet` and `SubscribedQuery`

`RefreshAsync_SkipsCollectedSubscribedGet`

`RefreshAsync_SkipsCollectedSubscribedQuery`

Why `[NoInlining]` is essential

Note on the `IDisposable` fix interaction