fix(plugins): close LA5 host lifecycle review

src/AcDream.Core/Plugins/LoadedPlugin.cs

@@ -7,14 +7,17 @@ namespace AcDream.Core.Plugins;
 /// Outcome of a plugin load attempt.
 /// <para>On success, <see cref="Plugin"/> is the instantiated plugin, <see cref="LoadContext"/>
 /// owns its assembly, and <see cref="Error"/> is null.</para>
-/// <para>On failure, <see cref="Plugin"/> and <see cref="LoadContext"/> are null and
-/// <see cref="Error"/> describes what went wrong.</para>
+/// <para>On failure, <see cref="Plugin"/> and <see cref="LoadContext"/> are null,
+/// <see cref="Error"/> describes what went wrong, and
+/// <see cref="ReleasedLoadContext"/> weakly observes any collectible context
+/// that was already released during rollback.</para>
 /// </summary>
 public sealed record LoadedPlugin(
     PluginManifest Manifest,
     IAcDreamPlugin? Plugin,
     AssemblyLoadContext? LoadContext,
-    Exception? Error)
+    Exception? Error,
+    WeakReference? ReleasedLoadContext = null)
 {
     public bool Success => Plugin is not null && Error is null;
 }

src/AcDream.Core/Plugins/PluginLoader.cs

@@ -48,13 +48,15 @@ public static class PluginLoader
 
             if (pluginType is null)
             {
+                var released = new WeakReference(alc);
                 alc.Unload();
                 return new LoadedPlugin(
                     manifest,
                     Plugin: null,
                     LoadContext: null,
                     Error: new InvalidOperationException(
-                        $"no IAcDreamPlugin implementation found in {manifest.EntryDll}"));
+                        $"no IAcDreamPlugin implementation found in {manifest.EntryDll}"),
+                    ReleasedLoadContext: released);
             }
 
             instance = (IAcDreamPlugin)Activator.CreateInstance(pluginType)!;
@@ -68,9 +70,15 @@ public static class PluginLoader
             // as an Enable failure before releasing the collectible context.
             try { instance?.Disable(); }
             catch { }
+            WeakReference? released = alc is null ? null : new WeakReference(alc);
             try { alc?.Unload(); }
             catch { }
-            return new LoadedPlugin(manifest, Plugin: null, LoadContext: null, Error: ex);
+            return new LoadedPlugin(
+                manifest,
+                Plugin: null,
+                LoadContext: null,
+                Error: ex,
+                ReleasedLoadContext: released);
         }
     }
 }

src/AcDream.Core/Plugins/PluginSession.cs

@@ -27,7 +27,8 @@ public sealed class PluginSession : IDisposable
 {
     private readonly IPluginHost _host;
     private readonly Action<PluginSessionStatus>? _report;
-    private readonly List<LoadedPlugin> _loaded = [];
+    private readonly List<ActivePlugin> _loaded = [];
+    private readonly List<WeakReference> _releasedContexts = [];
     private bool _started;
     private bool _disposed;
 
@@ -42,7 +43,7 @@ public sealed class PluginSession : IDisposable
     public int LoadedCount => _loaded.Count;
 
     public IReadOnlyList<string> LoadedPluginIds =>
-        _loaded.Select(static plugin => plugin.Manifest.Id).ToArray();
+        _loaded.Select(static active => active.Loaded.Manifest.Id).ToArray();
 
     /// <summary>
     /// Discovers and starts the configured set exactly once. A
@@ -143,9 +144,11 @@ public sealed class PluginSession : IDisposable
     /// owned by this session. The returned weak references do not delay unload.
     /// </summary>
     public IReadOnlyList<WeakReference> CaptureLoadContextWeakReferences() =>
-        _loaded
-            .Select(static plugin => new WeakReference(plugin.LoadContext!))
-            .ToArray();
+        [
+            .. _releasedContexts,
+            .. _loaded.Select(static active =>
+                new WeakReference(active.Loaded.LoadContext!)),
+        ];
 
     public void Dispose()
     {
@@ -155,7 +158,8 @@ public sealed class PluginSession : IDisposable
 
         for (int index = _loaded.Count - 1; index >= 0; index--)
         {
-            LoadedPlugin loaded = _loaded[index];
+            ActivePlugin active = _loaded[index];
+            LoadedPlugin loaded = active.Loaded;
             try
             {
                 loaded.Plugin!.Disable();
@@ -169,6 +173,11 @@ public sealed class PluginSession : IDisposable
                     error);
             }
 
+            // Host-owned registrations are released even when Disable throws.
+            // This must precede ALC unload so no UI binding or event delegate
+            // can keep the plugin assembly reachable.
+            active.Scope.Dispose();
+
             try
             {
                 loaded.LoadContext!.Unload();
@@ -198,12 +207,16 @@ public sealed class PluginSession : IDisposable
         {
             foreach (PluginDiscoveryResult candidate in available)
             {
+                var scope = new ScopedPluginHost(_host);
                 LoadedPlugin loaded = PluginLoader.Load(
                     candidate.PluginDirectory,
                     candidate.Manifest!,
-                    _host);
+                    scope);
                 if (!loaded.Success)
                 {
+                    scope.Dispose();
+                    if (loaded.ReleasedLoadContext is { } released)
+                        _releasedContexts.Add(released);
                     AddError(
                         errors,
                         id,
@@ -215,7 +228,7 @@ public sealed class PluginSession : IDisposable
                 try
                 {
                     loaded.Plugin!.Enable();
-                    _loaded.Add(loaded);
+                    _loaded.Add(new ActivePlugin(loaded, scope));
                     SafeLog(
                         static (log, message, _) => log.Info(message),
                         $"plugin loaded: {loaded.Manifest.Id} "
@@ -229,7 +242,7 @@ public sealed class PluginSession : IDisposable
                 catch (Exception error)
                 {
                     AddError(errors, id, error);
-                    ReleaseFailedEnable(loaded);
+                    ReleaseFailedEnable(loaded, scope);
                 }
             }
         }
@@ -257,7 +270,9 @@ public sealed class PluginSession : IDisposable
             null);
     }
 
-    private void ReleaseFailedEnable(LoadedPlugin loaded)
+    private void ReleaseFailedEnable(
+        LoadedPlugin loaded,
+        ScopedPluginHost scope)
     {
         try
         {
@@ -272,6 +287,9 @@ public sealed class PluginSession : IDisposable
                 error);
         }
 
+        scope.Dispose();
+
+        _releasedContexts.Add(new WeakReference(loaded.LoadContext!));
         try
         {
             loaded.LoadContext!.Unload();
@@ -358,4 +376,8 @@ public sealed class PluginSession : IDisposable
             or ArgumentException
             or NotSupportedException
             or System.Security.SecurityException;
+
+    private sealed record ActivePlugin(
+        LoadedPlugin Loaded,
+        ScopedPluginHost Scope);
 }

src/AcDream.Core/Plugins/ScopedPluginHost.cs

@@ -0,0 +1,171 @@
+using AcDream.Plugin.Abstractions;
+
+namespace AcDream.Core.Plugins;
+
+/// <summary>
+/// Per-plugin host view that owns every registration made through the public
+/// event/UI surfaces. Disposal is the host's rollback boundary: it removes
+/// registrations even when plugin Initialize/Enable/Disable code throws.
+/// </summary>
+internal sealed class ScopedPluginHost : IPluginHost, IDisposable
+{
+    private readonly IPluginHost _inner;
+    private readonly ScopedEvents _events;
+    private readonly ScopedUiRegistry _ui;
+    private bool _disposed;
+
+    internal ScopedPluginHost(IPluginHost inner)
+    {
+        _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+        _events = new ScopedEvents(inner.Events);
+        _ui = new ScopedUiRegistry(inner.Ui);
+    }
+
+    public bool HasUi => _inner.HasUi;
+    public IPluginLogger Log => _inner.Log;
+    public IGameState State => _inner.State;
+    public IEvents Events => _events;
+    public ISelectionService Selection => _inner.Selection;
+    public IUiRegistry Ui => _ui;
+
+    public void Dispose()
+    {
+        if (_disposed)
+            return;
+        _disposed = true;
+        _events.Dispose();
+        _ui.Dispose();
+    }
+
+    private sealed class ScopedEvents(IEvents inner) : IEvents, IDisposable
+    {
+        private readonly object _gate = new();
+        private readonly List<Action<WorldEntitySnapshot>> _registrations = [];
+        private bool _disposed;
+
+        public event Action<WorldEntitySnapshot> EntitySpawned
+        {
+            add
+            {
+                ArgumentNullException.ThrowIfNull(value);
+                try
+                {
+                    inner.EntitySpawned += value;
+                }
+                catch
+                {
+                    // A custom event source may mutate before its add accessor
+                    // faults. Best-effort removal keeps the scope transactional.
+                    try { inner.EntitySpawned -= value; }
+                    catch { }
+                    throw;
+                }
+                lock (_gate)
+                {
+                    if (!_disposed)
+                    {
+                        _registrations.Add(value);
+                        return;
+                    }
+                }
+
+                // Disposal may race the host subscription call. In that case
+                // the disposal snapshot could not see this registration, so
+                // the attaching thread must roll it back before returning.
+                try { inner.EntitySpawned -= value; }
+                catch { }
+                throw new ObjectDisposedException(nameof(ScopedEvents));
+            }
+            remove
+            {
+                if (value is null)
+                    return;
+                inner.EntitySpawned -= value;
+                lock (_gate)
+                    RemoveLast(value);
+            }
+        }
+
+        public void Dispose()
+        {
+            Action<WorldEntitySnapshot>[] registrations;
+            lock (_gate)
+            {
+                if (_disposed)
+                    return;
+                _disposed = true;
+                registrations = _registrations.ToArray();
+                _registrations.Clear();
+            }
+
+            for (int index = registrations.Length - 1; index >= 0; index--)
+            {
+                try { inner.EntitySpawned -= registrations[index]; }
+                catch { }
+            }
+        }
+
+        private void RemoveLast(Action<WorldEntitySnapshot> handler)
+        {
+            for (int index = _registrations.Count - 1; index >= 0; index--)
+            {
+                if (_registrations[index] != handler)
+                    continue;
+                _registrations.RemoveAt(index);
+                return;
+            }
+        }
+    }
+
+    private sealed class ScopedUiRegistry : IUiRegistry, IDisposable
+    {
+        private readonly IScopedUiRegistry _inner;
+        private readonly object _gate = new();
+        private readonly List<IDisposable> _registrations = [];
+        private bool _disposed;
+
+        internal ScopedUiRegistry(IUiRegistry inner)
+        {
+            _inner = inner as IScopedUiRegistry
+                ?? throw new InvalidOperationException(
+                    "Plugin hosts must expose an IScopedUiRegistry so UI registrations can be rolled back.");
+        }
+
+        public void AddMarkupPanel(string markupPath, object binding)
+        {
+            IDisposable registration = _inner.RegisterMarkupPanel(
+                markupPath,
+                binding);
+            lock (_gate)
+            {
+                if (!_disposed)
+                {
+                    _registrations.Add(registration);
+                    return;
+                }
+            }
+
+            registration.Dispose();
+            throw new ObjectDisposedException(nameof(ScopedUiRegistry));
+        }
+
+        public void Dispose()
+        {
+            IDisposable[] registrations;
+            lock (_gate)
+            {
+                if (_disposed)
+                    return;
+                _disposed = true;
+                registrations = _registrations.ToArray();
+                _registrations.Clear();
+            }
+
+            for (int index = registrations.Length - 1; index >= 0; index--)
+            {
+                try { registrations[index].Dispose(); }
+                catch { }
+            }
+        }
+    }
+}