The ALC drain is a subscription, not a spin against a deadline (#2488) by rbuergi · Pull Request #2549 · Systemorph/MeshWeaver

rbuergi · 2026-08-27T21:02:42Z

The defect

NodeAssemblyLoadContext.Dispose spun on _pins against a 5 s ceiling and then — by its own comment — "unload[ed] anyway (last-resort, matching the prior always-unload behaviour)".

So the one case where we know a scanner is still live (pins still held when the deadline expires) was also the case where its assembly got pulled out from under it: TypeLoadException '…format is invalid' mid GetTypes/attribute resolution — the #613 use-after-unload family, which surfaces far from its cause as a render that never completes or a delayed crash.

This is the same anti-pattern MessageHubGrain.OnDeactivateAsync had already removed, and /async Rule 1a names the cure: hang the work off the signal and let the turn return.

The fix

The last PinScope release announces through an AsyncSubject<Unit>; Dispose subscribes (.Take(1), fluent .Catch that keeps the context on a fault) and returns immediately. CompleteUnload() — close the door, Unload(), run the opt-in post-unload GC probe — is the callback.

No branch is left to get wrong: pins reach zero and the context unloads, or they do not and the context is retained. Retaining memory beats corrupting an assembly a hub is executing, and a drain that never completes is a leak to find, not a budget to spend. AsyncSubject (not a bare Subject) so a subscriber arriving after the last release still runs — the ordering hazard the inline path would otherwise have.

Tests

AlcDrainIsReactiveTest pins all three properties:

no scan in flight → unloads inline (the signal replays);
a scan pinned → does NOT unload, then unloads exactly on its release;
Pin() refused once Dispose started, so the drain can only shrink (otherwise "retain until quiet" becomes "retain forever").

Red-proven: restoring the spin loop fails the pinned case — and takes 10 s doing it, which is the ceiling being spent.

Scope

Sites 2 and 3 from the issue (MeshTeardownExtensions, MonolithMeshTestBase) already carry the reactive shape via ObserveCompletion; site 1 was the outstanding one.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Tz1i2fFcJhvR1Eje8yFH5F

NodeAssemblyLoadContext.Dispose spun on _pins against a 5 s ceiling and then, by its own comment, "unload[ed] anyway (last-resort, matching the prior always-unload behaviour)". That makes the one case where we KNOW a scanner is live — pins still held at the deadline — also the case where its assembly is pulled out from under it: TypeLoadException '…format is invalid' mid GetTypes/attribute resolution, the #613 use-after-unload family, surfacing far from the cause as a render that never completes. It is the exact anti-pattern MessageHubGrain.OnDeactivateAsync already had removed, and /async Rule 1a states the cure: hang the work off the signal and let the turn return. The last PinScope release announces through an AsyncSubject; Dispose subscribes (.Take(1), fluent .Catch that KEEPS the context on a fault) and returns immediately. CompleteUnload — close the door, Unload, run the opt-in post-unload GC probe — is the callback. There is no branch left to get wrong: pins reach zero and the context unloads, or they do not and it is RETAINED. Retaining memory beats corrupting an assembly a hub is executing, and a drain that never completes is a leak to find, not a budget to spend. AlcDrainIsReactiveTest pins all three properties (quiet → unloads inline; pinned → does NOT unload, then unloads on release; Pin refused once Dispose started, so the drain can only shrink). Red-proven: restoring the spin loop fails the pinned case, and takes 10 s doing it. Sites 2 and 3 of #2488 (MeshTeardownExtensions, MonolithMeshTestBase) already carry the reactive shape via ObserveCompletion; this was the outstanding one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tz1i2fFcJhvR1Eje8yFH5F

Copilot

Pull request overview

This PR changes the dynamic node AssemblyLoadContext teardown in MeshWeaver.Graph so that unloading is driven by a reactive drain signal (pin count reaching zero) rather than a time-bounded spin loop, reducing the risk of unloading an assembly while a reflection scan is still in flight (the use-after-unload family described in #613 / #2488).

Changes:

Replace the Dispose() pin-drain “spin until deadline then unload anyway” logic with an AsyncSubject<Unit> drain signal and a subscription that triggers the unload once the last pin releases.
Make the last pin release announce the drain completion so disposal can be event-driven.
Add AlcDrainIsReactiveTest to lock in the new behavior (immediate unload when unpinned, deferred unload while pinned, and pin refusal once disposal starts).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/MeshWeaver.Graph/Configuration/CompilationCacheService.cs`	Implements the reactive drain signal and subscription-based unload path for `NodeAssemblyLoadContext.Dispose()`.
`test/MeshWeaver.Graph.Test/AlcDrainIsReactiveTest.cs`	Adds regression tests asserting the subscription-driven drain/unload semantics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

A real defect in the fix, caught before merge. AnnounceDrained fired whenever pins hit zero — which happens constantly during normal operation — and AsyncSubject completion is PERMANENT. So a healthy context could complete its drain signal, and a Dispose arriving later WITH A NEW SCAN PINNED would subscribe to an already-completed subject, replay instantly, and unload with _pins > 0: the exact use-after-unload this change removes, reintroduced through the change itself. Guarded on _unloading. Once Dispose has started, Pin() refuses new scans, so the count can only shrink and zero really is the drain. AnEarlierScanReachingZero_DoesNotPreArmTheUnload pins the interleave (ordinary scan completes → new scan pins → Dispose → must NOT unload). Red-proven: it is the one test that fails with the guard removed. Also drops the reflection helper for the type's own IsDisposed, so a field rename cannot quietly turn this suite green against a broken implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tz1i2fFcJhvR1Eje8yFH5F

github-actions · 2026-08-27T21:38:28Z